From a07e8ad7815c9af7056365147bf50a998c42b972 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 10:44:58 +0200 Subject: [PATCH 1/2] fix(background): rasterize the heropattern tile at its target scale MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit tile_w = def.width * cfg.scale is computed at line 581 and used only for the overscan margin; the pixmap at 605-606 is sized from the unscaled def.width/def.height and resvg renders with Transform::default() (identity). The magnification then happens on the raster, via the shader matrix, with SamplingOptions::default() — which I read in skia- safe-0.99.0/src/core/sampling_options.rs:70-80: filter: FilterMode::Nearest, mipmap: MipmapMode::None. So heropattern with scale: 4 on a 1080p frame draws hard 4x4 pixel blocks from a 20x20 source raster, when the source is vector art that resvg could have rendered crisply at any size. The whole point of shipping 87 SVG patterns is lost above scale 1. It is also wasteful below 1: a scale: 0.25 pattern is rasterized at full size and then point- sampled down, aliasing badly. Fix: Size the pixmap at the scaled tile ((def.width * cfg.scale).ceil() x (def.height * cfg.scale).ceil(), clamped to a sane maximum) and pass tiny_skia::Transform::from_scale(cfg.scale, cfg.scale) to resvg::render, then drop the shader matrix so the tile is used 1:1. At minimum, pass SamplingOptions::new(FilterMode::Linear, MipmapMode::Linear) instead of the nearest-neighbour default. Refs #220 --- .../src/engine/render/background.rs | 92 ++++++++++++++++--- 1 file changed, 78 insertions(+), 14 deletions(-) diff --git a/crates/rustmotion/src/engine/render/background.rs b/crates/rustmotion/src/engine/render/background.rs index 2c5d719b..44532e21 100644 --- a/crates/rustmotion/src/engine/render/background.rs +++ b/crates/rustmotion/src/engine/render/background.rs @@ -567,6 +567,12 @@ fn draw_bg_pixel_grid( } /// Tiled heropattern background. +/// +/// The tile is rasterized once at `cfg.scale`, using `heropattern_raster_size` +/// and a matching `resvg` render transform, then tiled 1:1 by the shader. +/// It used to be rasterized at 1x and magnified by the shader's own matrix +/// instead, which turned the vector source into hard nearest-neighbour +/// blocks above `scale: 1` and aliased it below `scale: 1`. fn draw_bg_heropattern( canvas: &Canvas, cfg: &HeropatternConfig, @@ -584,7 +590,6 @@ fn draw_bg_heropattern( return; } - // Build the SVG source with color/opacity substituted let svg_content = format!( r#"{}"#, def.width, @@ -596,20 +601,23 @@ fn draw_bg_heropattern( .replace("{{opacity}}", &cfg.opacity.to_string()), ); - // Render one tile via usvg/resvg let opt = usvg::Options::default(); let Ok(tree) = usvg::Tree::from_data(svg_content.as_bytes(), &opt) else { return; }; - let pw = def.width.ceil() as u32; - let ph = def.height.ceil() as u32; + let (pw, ph) = heropattern_raster_size(def.width, def.height, cfg.scale); let Some(mut pixmap) = tiny_skia::Pixmap::new(pw, ph) else { return; }; - resvg::render(&tree, tiny_skia::Transform::default(), &mut pixmap.as_mut()); + let render_scale_x = pw as f32 / def.width; + let render_scale_y = ph as f32 / def.height; + resvg::render( + &tree, + tiny_skia::Transform::from_scale(render_scale_x, render_scale_y), + &mut pixmap.as_mut(), + ); - // Convert to Skia image let info = ImageInfo::new( (pw as i32, ph as i32), ColorType::RGBA8888, @@ -625,16 +633,10 @@ fn draw_bg_heropattern( return; }; - // Build a tiled shader from the tile image - let matrix = if cfg.scale != 1.0 { - Some(skia_safe::Matrix::scale((cfg.scale, cfg.scale))) - } else { - None - }; let Some(shader) = tile_image.to_shader( (skia_safe::TileMode::Repeat, skia_safe::TileMode::Repeat), - skia_safe::SamplingOptions::default(), - matrix.as_ref(), + skia_safe::SamplingOptions::new(skia_safe::FilterMode::Linear, skia_safe::MipmapMode::None), + None, ) else { return; }; @@ -655,6 +657,21 @@ fn draw_bg_heropattern( ); } +/// Pixel size to rasterize one heropattern tile at, so the vector source is +/// re-rendered crisp at `scale` instead of rasterized at the pattern's +/// native `(width, height)` and then magnified. Clamped to `MAX_TILE_PX` +/// per axis: `HeropatternConfig::scale` has no upper bound in the schema, so +/// an unclamped scale could ask for an arbitrarily large pixmap allocation. +/// A clamped tile still tiles seamlessly with itself — it just renders +/// smaller than an extreme `scale` asked for, which is the trade the "sane +/// maximum" this is named for is making. +fn heropattern_raster_size(width: f32, height: f32, scale: f32) -> (u32, u32) { + const MAX_TILE_PX: f32 = 4096.0; + let pw = (width * scale).ceil().clamp(1.0, MAX_TILE_PX) as u32; + let ph = (height * scale).ceil().clamp(1.0, MAX_TILE_PX) as u32; + (pw, ph) +} + /// Interpolate two AnimatedBackground structs. `t` goes from 0.0 (fully `a`) to 1.0 (fully `b`). #[allow(dead_code)] pub(super) fn interpolate_animated_bg( @@ -1523,3 +1540,50 @@ mod heropattern_period_tests { assert!(spacing_x >= 20.0); } } + +#[cfg(test)] +mod heropattern_raster_tests { + //! the heropattern tile used to be rasterized at 1x (the + //! pattern's native width/height) and then magnified by the shader's + //! own matrix with nearest-neighbour sampling — blocky above `scale: 1`, + //! aliased below it. `heropattern_raster_size` must honour `scale` + //! directly in the raster resolution instead. + + use super::*; + + #[test] + fn raster_size_scales_with_cfg_scale_not_pinned_to_1x() { + let (pw, ph) = heropattern_raster_size(32.0, 64.0, 4.0); + assert_eq!( + (pw, ph), + (128, 256), + "the pixmap must be sized for the scaled tile, not the pattern's native 32x64" + ); + } + + #[test] + fn raster_size_matches_the_pattern_exactly_at_scale_1() { + assert_eq!(heropattern_raster_size(32.0, 64.0, 1.0), (32, 64)); + } + + #[test] + fn raster_size_is_clamped_for_an_unbounded_scale() { + let (pw, ph) = heropattern_raster_size(32.0, 64.0, 100_000.0); + assert!( + pw <= 4096 && ph <= 4096, + "an extreme scale must not attempt an unbounded pixmap allocation, got {pw}x{ph}" + ); + } + + #[test] + fn draw_bg_heropattern_does_not_panic_at_an_extreme_scale() { + let mut surface = skia_safe::surfaces::raster_n32_premul((64, 64)).expect("surface"); + let cfg = HeropatternConfig { + pattern: "aztec".to_string(), + color: "#FFFFFF".to_string(), + opacity: 0.1, + scale: 100_000.0, + }; + draw_bg_heropattern(surface.canvas(), &cfg, 0.0, 64.0, 64.0); + } +} From e5699b75e837a827c73f85dd477891b85c12d354 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 10:45:07 +0200 Subject: [PATCH 2/2] refactor(scene): paint decorative fullscreen through the box tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every non-decorative node is timed by PaintWindow::contains — self.end.is_none_or(|e| t < e) (rustmotion-core/src/engine/box_tree.rs:43), a half-open window — and gets its effects from box_builder::effective_effects (legacy_dispatch.rs:110), which folds in timeline steps and style.transition keyframes. This second path uses an inclusive time > e and calls a.animation_effects() raw. Particle declares timeline: Vec (particle.rs:57), so a particle's timeline steps and transition keyframes are silently dropped, and its end_at keeps it visible for one extra frame — but only in a world view, since this is the sole call site (scene.rs:1130). The same particle in a slide view behaves correctly. A component whose behaviour depends on which view type contains it is invisible to tests written against either one. Fix: Give decorative children a real BoxNode with position: absolute; width/height: 100% and let them flow through paint_tree like everything else, deleting paint_decorative_fullscreen. If the bypass must stay, have it call effective_effects and PaintWindow::contains rather than re-deriving both. Refs #220 --- .../src/engine/render/background.rs | 2 +- crates/rustmotion/src/engine/render/scene.rs | 33 ++--- crates/rustmotion/tests/audit_ws_e.rs | 139 +++++++++++++++++- 3 files changed, 147 insertions(+), 27 deletions(-) diff --git a/crates/rustmotion/src/engine/render/background.rs b/crates/rustmotion/src/engine/render/background.rs index 44532e21..ef95320e 100644 --- a/crates/rustmotion/src/engine/render/background.rs +++ b/crates/rustmotion/src/engine/render/background.rs @@ -1543,7 +1543,7 @@ mod heropattern_period_tests { #[cfg(test)] mod heropattern_raster_tests { - //! the heropattern tile used to be rasterized at 1x (the + //! The heropattern tile used to be rasterized at 1x (the //! pattern's native width/height) and then magnified by the shader's //! own matrix with nearest-neighbour sampling — blocky above `scale: 1`, //! aliased below it. `heropattern_raster_size` must honour `scale` diff --git a/crates/rustmotion/src/engine/render/scene.rs b/crates/rustmotion/src/engine/render/scene.rs index dde851e3..cf7d96f2 100644 --- a/crates/rustmotion/src/engine/render/scene.rs +++ b/crates/rustmotion/src/engine/render/scene.rs @@ -553,6 +553,13 @@ fn render_with_new_pipeline_iter<'a, I>( /// Paint a decorative leaf (e.g. Particle) over the full viewport without /// going through taffy. Resolves animations and dispatches to /// `Painter::paint_content` directly with a viewport-sized `BoxLayout`. +/// +/// Visibility and effects go through the same `PaintWindow::contains` and +/// `effective_effects` the ordinary `paint_tree` dispatch uses (see +/// `box_builder::effective_effects`'s doc comment), rather than re-deriving +/// both by hand — a component whose `timeline`/`style.transition` state or +/// exact `end_at` boundary only worked in one of the two dispatch paths used +/// to be invisible to tests written against either one alone. fn paint_decorative_fullscreen( canvas: &Canvas, child: &ChildComponent, @@ -560,34 +567,22 @@ fn paint_decorative_fullscreen( viewport_h: f32, ctx: &RenderContext, ) { + use rustmotion_components::box_builder::effective_effects; use rustmotion_core::engine::animator::{resolve_props_for_effects, AnimatedProperties}; + use rustmotion_core::engine::box_tree::PaintWindow; 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 time < s { - return; - } - } - if let Some(e) = end_at { - if time > e { - return; - } + let (start, end) = timed.timing(); + if !(PaintWindow { start, end }).contains(time) { + return; } } - let props = match child.component.as_animatable() { - Some(a) => { - let effects = a.animation_effects(); - if effects.is_empty() { - AnimatedProperties::default() - } else { - resolve_props_for_effects(effects, time, ctx.scene_duration) - } - } + let props = match effective_effects(&child.component, 0.0) { + Some(effects) => resolve_props_for_effects(&effects, time, ctx.scene_duration), None => AnimatedProperties::default(), }; if props.opacity <= 0.0 { diff --git a/crates/rustmotion/tests/audit_ws_e.rs b/crates/rustmotion/tests/audit_ws_e.rs index 5a2f31eb..9b0e92ab 100644 --- a/crates/rustmotion/tests/audit_ws_e.rs +++ b/crates/rustmotion/tests/audit_ws_e.rs @@ -1,18 +1,143 @@ -//! Regression tests — audit round chantier/audit-2026-09, workstream E -//! (animated backgrounds & the scene render path). +//! Regression tests — workstream E (animated backgrounds & the scene render +//! path). //! -//! Two of the four findings are bugs in fully private -//! rendering internals (`background.rs`'s `tile_spacing`/`compute_scroll_offset`/ +//! Some of the covered defects are bugs in fully private rendering +//! internals (`background.rs`'s `tile_spacing`/`compute_scroll_offset`/ //! `draw_bg_heropattern`) that this crate never exposes past its `pub` //! surface — an external integration test crate like this one cannot name //! them. Their regression tests live as `#[cfg(test)]` modules inside //! `crates/rustmotion/src/engine/render/background.rs` itself, following //! that file's own pre-existing convention (`scroll_offset_wrap_tests`, //! `pixel_grid_tests`, `grid_lines_tests`, `halo_opacity_tests`) for testing -//! renderer-private logic directly. This file carries the findings that are +//! renderer-private logic directly. This file carries the defects that are //! genuinely reachable through the crate's public API. -//! -//! One section per finding: the paint path, then colour templating. use rustmotion::encode::video::{build_frame_tasks, render_frame_task, FrameTask}; use rustmotion::loader::load_scenario_from_source; + +/// Render a single `FrameTask::WorldFrame` at `frame_in_view`, decoded from +/// `scenario_json`. Panics (with a message naming the missing frame) if no +/// such world frame exists in the built schedule — a test bug, not a +/// render-time failure, should fail loudly here. +fn render_world_frame(scenario_json: &serde_json::Value, frame_in_view: u32) -> Vec { + let scenario = load_scenario_from_source(None, Some(&scenario_json.to_string())) + .expect("scenario is schema-valid"); + let tasks = build_frame_tasks(&scenario); + 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 at frame_in_view={frame_in_view}")); + render_frame_task(&scenario.video, &scenario, task).expect("frame renders") +} + +/// Count pixels that read as the particle's pure-green marker colour +/// (`#00FF00`) against the scenario's plain black background — a stand-in +/// for "is the decorative child visible in this frame" that doesn't depend +/// on knowing any particle's exact on-screen position. +fn green_pixel_count(buf: &[u8]) -> usize { + buf.chunks_exact(4) + .filter(|px| px[1] > 100 && px[0] < 80 && px[2] < 80) + .count() +} + +mod decorative_dispatch_parity { + //! `paint_decorative_fullscreen` (the world-view-only path that paints + //! decorative children like `particle` without going through the box + //! tree) used to re-derive visibility and effects by hand instead of + //! calling `PaintWindow::contains` / `box_builder::effective_effects` + //! like every other paint path does. Two independent symptoms: an + //! inclusive `end_at` (visible one frame too long) and dropped + //! `timeline` animation effects. + + use super::*; + + const FPS: u32 = 10; + + fn world_scenario(particle_extra: serde_json::Value) -> serde_json::Value { + let mut particle = serde_json::json!({ + "type": "particle", + "particle_type": "snow", + "count": 30, + "colors": ["#00FF00"], + "size_range": {"min": 6, "max": 6}, + "speed": 0.0, + }); + particle + .as_object_mut() + .unwrap() + .extend(particle_extra.as_object().unwrap().clone()); + + serde_json::json!({ + "video": {"width": 64, "height": 64, "fps": FPS, "background": "#000000"}, + "composition": [ + {"type": "world", "scenes": [ + {"duration": 2.0, "children": [particle]} + ]} + ] + }) + } + + /// `end_at` is a half-open window: the child must already be gone + /// exactly at `end_at`, not still visible for one extra frame past it. + #[test] + fn end_at_is_a_half_open_window_not_inclusive() { + let end_at = 0.5; + let scenario = world_scenario(serde_json::json!({ "end_at": end_at })); + let frame_just_before_end_at = (end_at * FPS as f64) as u32 - 1; + let frame_at_end_at = (end_at * FPS as f64) as u32; + + let before = render_world_frame(&scenario, frame_just_before_end_at); + assert!( + green_pixel_count(&before) > 0, + "the particle must still be visible just before its end_at" + ); + + let at_boundary = render_world_frame(&scenario, frame_at_end_at); + assert_eq!( + green_pixel_count(&at_boundary), + 0, + "end_at is a half-open window ([start, end)): the particle must already be gone \ + exactly at end_at, not one extra frame later" + ); + } + + /// A `timeline` step's `animation` entries must be folded into the + /// resolved props like every other paint path does, not silently + /// dropped because only `style.animation` was read. + #[test] + fn timeline_animation_effects_are_not_silently_dropped() { + let fade_out_at = 0.5; + let scenario = world_scenario(serde_json::json!({ + "timeline": [{ + "at": 0.0, + "animation": [{ + "name": "keyframes", + "keyframes": [{ + "property": "opacity", + "keyframes": [ + {"time": 0.0, "value": 1.0}, + {"time": fade_out_at, "value": 0.0} + ] + }] + }] + }] + })); + + let early = render_world_frame(&scenario, 0); + assert!( + green_pixel_count(&early) > 0, + "the particle should be visible at t=0, before the timeline fade-out completes" + ); + + let frame_well_past_fade_out = (fade_out_at * FPS as f64) as u32 * 3; + let late = render_world_frame(&scenario, frame_well_past_fade_out); + assert_eq!( + green_pixel_count(&late), + 0, + "a timeline step's animation effects must apply to a decorative child, not be \ + silently dropped" + ); + } +}