Skip to content

refactor(scene): paint decorative fullscreen through the box tree - #249

Closed
LeadcodeDev wants to merge 2 commits into
fix/heropattern-scroll-periodfrom
refactor/decorative-paint-path
Closed

LeadcodeDev wants to merge 2 commits into
fix/heropattern-scroll-periodfrom
refactor/decorative-paint-path

Conversation

@LeadcodeDev

Copy link
Copy Markdown
Owner

Severity Low, category correctness. Location: crates/rustmotion/src/engine/render/scene.rs:595

Impact

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.

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.

Evidence the audit read

if let Some(e) = end_at {
            if time > e { 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) }

Stacked on fix/heropattern-scroll-period, which carries the previous finding of this workstream. GitHub shows only this finding's diff; merge in order.

Part of the September 2026 audit remediation chantier. Refs #220 (RM-36).

@LeadcodeDev LeadcodeDev added the bug Something isn't working label Sep 21, 2026
@LeadcodeDev LeadcodeDev self-assigned this Sep 21, 2026
@LeadcodeDev
LeadcodeDev force-pushed the fix/heropattern-scroll-period branch from 4c0b4ae to f067906 Compare September 22, 2026 06:10
@LeadcodeDev
LeadcodeDev force-pushed the refactor/decorative-paint-path branch from 3dcfc14 to afbca7b Compare September 22, 2026 06:10
@LeadcodeDev
LeadcodeDev force-pushed the fix/heropattern-scroll-period branch from f067906 to 67be435 Compare September 22, 2026 08:35
@LeadcodeDev
LeadcodeDev force-pushed the refactor/decorative-paint-path branch from afbca7b to 6a2f28e Compare September 22, 2026 08:35
@LeadcodeDev
LeadcodeDev force-pushed the fix/heropattern-scroll-period branch from 67be435 to 5a7f871 Compare September 22, 2026 08:44
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
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.

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
@LeadcodeDev
LeadcodeDev force-pushed the refactor/decorative-paint-path branch from 6a2f28e to e5699b7 Compare September 22, 2026 08:45
@LeadcodeDev

Copy link
Copy Markdown
Owner Author

Superseded by #283. The workstream's finding order was declared wrong in the carving table, so this pull request was opened on the wrong base. #283 carries the same finding on the correct one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant