Skip to content

Commit d199738

Browse files
committed
refactor(scene): paint decorative fullscreen through the box tree
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<TimelineStep>` (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. Refs #220
1 parent 41689b8 commit d199738

3 files changed

Lines changed: 224 additions & 40 deletions

File tree

‎crates/rustmotion/src/engine/render/background.rs‎

Lines changed: 78 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -567,6 +567,12 @@ fn draw_bg_pixel_grid(
567567
}
568568

569569
/// Tiled heropattern background.
570+
///
571+
/// The tile is rasterized once at `cfg.scale`, using `heropattern_raster_size`
572+
/// and a matching `resvg` render transform, then tiled 1:1 by the shader.
573+
/// It used to be rasterized at 1x and magnified by the shader's own matrix
574+
/// instead, which turned the vector source into hard nearest-neighbour
575+
/// blocks above `scale: 1` and aliased it below `scale: 1`.
570576
fn draw_bg_heropattern(
571577
canvas: &Canvas,
572578
cfg: &HeropatternConfig,
@@ -584,7 +590,6 @@ fn draw_bg_heropattern(
584590
return;
585591
}
586592

587-
// Build the SVG source with color/opacity substituted
588593
let svg_content = format!(
589594
r#"<svg xmlns="http://www.w3.org/2000/svg" width="{}" height="{}" viewBox="0 0 {} {}">{}</svg>"#,
590595
def.width,
@@ -596,20 +601,23 @@ fn draw_bg_heropattern(
596601
.replace("{{opacity}}", &cfg.opacity.to_string()),
597602
);
598603

599-
// Render one tile via usvg/resvg
600604
let opt = usvg::Options::default();
601605
let Ok(tree) = usvg::Tree::from_data(svg_content.as_bytes(), &opt) else {
602606
return;
603607
};
604608

605-
let pw = def.width.ceil() as u32;
606-
let ph = def.height.ceil() as u32;
609+
let (pw, ph) = heropattern_raster_size(def.width, def.height, cfg.scale);
607610
let Some(mut pixmap) = tiny_skia::Pixmap::new(pw, ph) else {
608611
return;
609612
};
610-
resvg::render(&tree, tiny_skia::Transform::default(), &mut pixmap.as_mut());
613+
let render_scale_x = pw as f32 / def.width;
614+
let render_scale_y = ph as f32 / def.height;
615+
resvg::render(
616+
&tree,
617+
tiny_skia::Transform::from_scale(render_scale_x, render_scale_y),
618+
&mut pixmap.as_mut(),
619+
);
611620

612-
// Convert to Skia image
613621
let info = ImageInfo::new(
614622
(pw as i32, ph as i32),
615623
ColorType::RGBA8888,
@@ -625,16 +633,10 @@ fn draw_bg_heropattern(
625633
return;
626634
};
627635

628-
// Build a tiled shader from the tile image
629-
let matrix = if cfg.scale != 1.0 {
630-
Some(skia_safe::Matrix::scale((cfg.scale, cfg.scale)))
631-
} else {
632-
None
633-
};
634636
let Some(shader) = tile_image.to_shader(
635637
(skia_safe::TileMode::Repeat, skia_safe::TileMode::Repeat),
636-
skia_safe::SamplingOptions::default(),
637-
matrix.as_ref(),
638+
skia_safe::SamplingOptions::new(skia_safe::FilterMode::Linear, skia_safe::MipmapMode::None),
639+
None,
638640
) else {
639641
return;
640642
};
@@ -655,6 +657,21 @@ fn draw_bg_heropattern(
655657
);
656658
}
657659

660+
/// Pixel size to rasterize one heropattern tile at, so the vector source is
661+
/// re-rendered crisp at `scale` instead of rasterized at the pattern's
662+
/// native `(width, height)` and then magnified. Clamped to `MAX_TILE_PX`
663+
/// per axis: `HeropatternConfig::scale` has no upper bound in the schema, so
664+
/// an unclamped scale could ask for an arbitrarily large pixmap allocation.
665+
/// A clamped tile still tiles seamlessly with itself — it just renders
666+
/// smaller than an extreme `scale` asked for, which is the trade the "sane
667+
/// maximum" this is named for is making.
668+
fn heropattern_raster_size(width: f32, height: f32, scale: f32) -> (u32, u32) {
669+
const MAX_TILE_PX: f32 = 4096.0;
670+
let pw = (width * scale).ceil().clamp(1.0, MAX_TILE_PX) as u32;
671+
let ph = (height * scale).ceil().clamp(1.0, MAX_TILE_PX) as u32;
672+
(pw, ph)
673+
}
674+
658675
/// Interpolate two AnimatedBackground structs. `t` goes from 0.0 (fully `a`) to 1.0 (fully `b`).
659676
#[allow(dead_code)]
660677
pub(super) fn interpolate_animated_bg(
@@ -1523,3 +1540,50 @@ mod heropattern_period_tests {
15231540
assert!(spacing_x >= 20.0);
15241541
}
15251542
}
1543+
1544+
#[cfg(test)]
1545+
mod heropattern_raster_tests {
1546+
//! The heropattern tile used to be rasterized at 1x (the
1547+
//! pattern's native width/height) and then magnified by the shader's
1548+
//! own matrix with nearest-neighbour sampling — blocky above `scale: 1`,
1549+
//! aliased below it. `heropattern_raster_size` must honour `scale`
1550+
//! directly in the raster resolution instead.
1551+
1552+
use super::*;
1553+
1554+
#[test]
1555+
fn raster_size_scales_with_cfg_scale_not_pinned_to_1x() {
1556+
let (pw, ph) = heropattern_raster_size(32.0, 64.0, 4.0);
1557+
assert_eq!(
1558+
(pw, ph),
1559+
(128, 256),
1560+
"the pixmap must be sized for the scaled tile, not the pattern's native 32x64"
1561+
);
1562+
}
1563+
1564+
#[test]
1565+
fn raster_size_matches_the_pattern_exactly_at_scale_1() {
1566+
assert_eq!(heropattern_raster_size(32.0, 64.0, 1.0), (32, 64));
1567+
}
1568+
1569+
#[test]
1570+
fn raster_size_is_clamped_for_an_unbounded_scale() {
1571+
let (pw, ph) = heropattern_raster_size(32.0, 64.0, 100_000.0);
1572+
assert!(
1573+
pw <= 4096 && ph <= 4096,
1574+
"an extreme scale must not attempt an unbounded pixmap allocation, got {pw}x{ph}"
1575+
);
1576+
}
1577+
1578+
#[test]
1579+
fn draw_bg_heropattern_does_not_panic_at_an_extreme_scale() {
1580+
let mut surface = skia_safe::surfaces::raster_n32_premul((64, 64)).expect("surface");
1581+
let cfg = HeropatternConfig {
1582+
pattern: "aztec".to_string(),
1583+
color: "#FFFFFF".to_string(),
1584+
opacity: 0.1,
1585+
scale: 100_000.0,
1586+
};
1587+
draw_bg_heropattern(surface.canvas(), &cfg, 0.0, 64.0, 64.0);
1588+
}
1589+
}

‎crates/rustmotion/src/engine/render/scene.rs‎

Lines changed: 14 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -553,41 +553,36 @@ fn render_with_new_pipeline_iter<'a, I>(
553553
/// Paint a decorative leaf (e.g. Particle) over the full viewport without
554554
/// going through taffy. Resolves animations and dispatches to
555555
/// `Painter::paint_content` directly with a viewport-sized `BoxLayout`.
556+
///
557+
/// Visibility and effects go through the same `PaintWindow::contains` and
558+
/// `effective_effects` the ordinary `paint_tree` dispatch uses (see
559+
/// `box_builder::effective_effects`'s doc comment), rather than re-deriving
560+
/// both by hand — a component whose `timeline`/`style.transition` state or
561+
/// exact `end_at` boundary only worked in one of the two dispatch paths used
562+
/// to be invisible to tests written against either one alone.
556563
fn paint_decorative_fullscreen(
557564
canvas: &Canvas,
558565
child: &ChildComponent,
559566
viewport_w: f32,
560567
viewport_h: f32,
561568
ctx: &RenderContext,
562569
) {
570+
use rustmotion_components::box_builder::effective_effects;
563571
use rustmotion_core::engine::animator::{resolve_props_for_effects, AnimatedProperties};
572+
use rustmotion_core::engine::box_tree::PaintWindow;
564573
use rustmotion_core::engine::layout_pass::BoxLayout;
565574
use rustmotion_core::traits::PaintCtx;
566575

567576
let time = ctx.time.seconds();
568577
if let Some(timed) = child.component.as_timed() {
569-
let (start_at, end_at) = timed.timing();
570-
if let Some(s) = start_at {
571-
if time < s {
572-
return;
573-
}
574-
}
575-
if let Some(e) = end_at {
576-
if time > e {
577-
return;
578-
}
578+
let (start, end) = timed.timing();
579+
if !(PaintWindow { start, end }).contains(time) {
580+
return;
579581
}
580582
}
581583

582-
let props = match child.component.as_animatable() {
583-
Some(a) => {
584-
let effects = a.animation_effects();
585-
if effects.is_empty() {
586-
AnimatedProperties::default()
587-
} else {
588-
resolve_props_for_effects(effects, time, ctx.scene_duration)
589-
}
590-
}
584+
let props = match effective_effects(&child.component, 0.0) {
585+
Some(effects) => resolve_props_for_effects(&effects, time, ctx.scene_duration),
591586
None => AnimatedProperties::default(),
592587
};
593588
if props.opacity <= 0.0 {
Lines changed: 132 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,143 @@
1-
//! Regression tests — audit round chantier/audit-2026-09, workstream E
2-
//! (animated backgrounds & the scene render path).
1+
//! Regression tests — workstream E (animated backgrounds & the scene render
2+
//! path).
33
//!
4-
//! Two of the four findings are bugs in fully private
5-
//! rendering internals (`background.rs`'s `tile_spacing`/`compute_scroll_offset`/
4+
//! Some of the covered defects are bugs in fully private rendering
5+
//! internals (`background.rs`'s `tile_spacing`/`compute_scroll_offset`/
66
//! `draw_bg_heropattern`) that this crate never exposes past its `pub`
77
//! surface — an external integration test crate like this one cannot name
88
//! them. Their regression tests live as `#[cfg(test)]` modules inside
99
//! `crates/rustmotion/src/engine/render/background.rs` itself, following
1010
//! that file's own pre-existing convention (`scroll_offset_wrap_tests`,
1111
//! `pixel_grid_tests`, `grid_lines_tests`, `halo_opacity_tests`) for testing
12-
//! renderer-private logic directly. This file carries the findings that are
12+
//! renderer-private logic directly. This file carries the defects that are
1313
//! genuinely reachable through the crate's public API.
14-
//!
15-
//! One section per finding: the paint path, then colour templating.
1614
1715
use rustmotion::encode::video::{build_frame_tasks, render_frame_task, FrameTask};
1816
use rustmotion::loader::load_scenario_from_source;
17+
18+
/// Render a single `FrameTask::WorldFrame` at `frame_in_view`, decoded from
19+
/// `scenario_json`. Panics (with a message naming the missing frame) if no
20+
/// such world frame exists in the built schedule — a test bug, not a
21+
/// render-time failure, should fail loudly here.
22+
fn render_world_frame(scenario_json: &serde_json::Value, frame_in_view: u32) -> Vec<u8> {
23+
let scenario = load_scenario_from_source(None, Some(&scenario_json.to_string()))
24+
.expect("scenario is schema-valid");
25+
let tasks = build_frame_tasks(&scenario);
26+
let task = tasks
27+
.iter()
28+
.find(
29+
|t| matches!(t, FrameTask::WorldFrame { frame_in_view: f, .. } if *f == frame_in_view),
30+
)
31+
.unwrap_or_else(|| panic!("no WorldFrame task at frame_in_view={frame_in_view}"));
32+
render_frame_task(&scenario.video, &scenario, task).expect("frame renders")
33+
}
34+
35+
/// Count pixels that read as the particle's pure-green marker colour
36+
/// (`#00FF00`) against the scenario's plain black background — a stand-in
37+
/// for "is the decorative child visible in this frame" that doesn't depend
38+
/// on knowing any particle's exact on-screen position.
39+
fn green_pixel_count(buf: &[u8]) -> usize {
40+
buf.chunks_exact(4)
41+
.filter(|px| px[1] > 100 && px[0] < 80 && px[2] < 80)
42+
.count()
43+
}
44+
45+
mod decorative_dispatch_parity {
46+
//! `paint_decorative_fullscreen` (the world-view-only path that paints
47+
//! decorative children like `particle` without going through the box
48+
//! tree) used to re-derive visibility and effects by hand instead of
49+
//! calling `PaintWindow::contains` / `box_builder::effective_effects`
50+
//! like every other paint path does. Two independent symptoms: an
51+
//! inclusive `end_at` (visible one frame too long) and dropped
52+
//! `timeline` animation effects.
53+
54+
use super::*;
55+
56+
const FPS: u32 = 10;
57+
58+
fn world_scenario(particle_extra: serde_json::Value) -> serde_json::Value {
59+
let mut particle = serde_json::json!({
60+
"type": "particle",
61+
"particle_type": "snow",
62+
"count": 30,
63+
"colors": ["#00FF00"],
64+
"size_range": {"min": 6, "max": 6},
65+
"speed": 0.0,
66+
});
67+
particle
68+
.as_object_mut()
69+
.unwrap()
70+
.extend(particle_extra.as_object().unwrap().clone());
71+
72+
serde_json::json!({
73+
"video": {"width": 64, "height": 64, "fps": FPS, "background": "#000000"},
74+
"composition": [
75+
{"type": "world", "scenes": [
76+
{"duration": 2.0, "children": [particle]}
77+
]}
78+
]
79+
})
80+
}
81+
82+
/// `end_at` is a half-open window: the child must already be gone
83+
/// exactly at `end_at`, not still visible for one extra frame past it.
84+
#[test]
85+
fn end_at_is_a_half_open_window_not_inclusive() {
86+
let end_at = 0.5;
87+
let scenario = world_scenario(serde_json::json!({ "end_at": end_at }));
88+
let frame_just_before_end_at = (end_at * FPS as f64) as u32 - 1;
89+
let frame_at_end_at = (end_at * FPS as f64) as u32;
90+
91+
let before = render_world_frame(&scenario, frame_just_before_end_at);
92+
assert!(
93+
green_pixel_count(&before) > 0,
94+
"the particle must still be visible just before its end_at"
95+
);
96+
97+
let at_boundary = render_world_frame(&scenario, frame_at_end_at);
98+
assert_eq!(
99+
green_pixel_count(&at_boundary),
100+
0,
101+
"end_at is a half-open window ([start, end)): the particle must already be gone \
102+
exactly at end_at, not one extra frame later"
103+
);
104+
}
105+
106+
/// A `timeline` step's `animation` entries must be folded into the
107+
/// resolved props like every other paint path does, not silently
108+
/// dropped because only `style.animation` was read.
109+
#[test]
110+
fn timeline_animation_effects_are_not_silently_dropped() {
111+
let fade_out_at = 0.5;
112+
let scenario = world_scenario(serde_json::json!({
113+
"timeline": [{
114+
"at": 0.0,
115+
"animation": [{
116+
"name": "keyframes",
117+
"keyframes": [{
118+
"property": "opacity",
119+
"keyframes": [
120+
{"time": 0.0, "value": 1.0},
121+
{"time": fade_out_at, "value": 0.0}
122+
]
123+
}]
124+
}]
125+
}]
126+
}));
127+
128+
let early = render_world_frame(&scenario, 0);
129+
assert!(
130+
green_pixel_count(&early) > 0,
131+
"the particle should be visible at t=0, before the timeline fade-out completes"
132+
);
133+
134+
let frame_well_past_fade_out = (fade_out_at * FPS as f64) as u32 * 3;
135+
let late = render_world_frame(&scenario, frame_well_past_fade_out);
136+
assert_eq!(
137+
green_pixel_count(&late),
138+
0,
139+
"a timeline step's animation effects must apply to a decorative child, not be \
140+
silently dropped"
141+
);
142+
}
143+
}

0 commit comments

Comments
 (0)