From 384ad4e78ddcd4968b07515ca81e96d4311397b9 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sun, 9 Aug 2026 01:20:33 +0200 Subject: [PATCH] fix(paint): unbreak shadows, bound the opacity layer, make the banding fix real MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven confirmed audit findings in the paint pass, plus one regression guard for a side effect the scroll-wrap fix would otherwise have introduced. - `overflow: hidden` erased the node's own outset `box-shadow`: the clip was installed before the shadow was drawn, so the shadow was clipped away by the very box it was supposed to sit outside. - `backdrop-filter` was neutralised whenever `opacity < 1` on the same node. - The opacity `save_layer` was allocated with no bounds — one full-viewport layer per node. On a 60-frame scenario this dominated everything else: 42-60s down to 0.5s. - Leaf painters were handed the border-box origin with the padding insets zeroed, so every leaf ignored `padding`. `Codeblock` stays a deliberate, now-documented exception: it reads `style.padding` itself and paints its own background from the border box, so honouring the general contract for it too would double-apply padding. - Transform percentages were resolved against `max(width, height)` on both axes instead of per-axis. - Scrolling tiled backgrounds walked out of frame: the offset grew linearly with time forever while the draw loops only overscan by one tile, so the pattern left a growing blank band. The offset now wraps into one tile period, and `draw_bg_grid_dots`'s x-loop overscans symmetrically like its y-loop already did (it started at 0, with no left margin). - Both documented gradient banding mitigations were inert. The render surfaces are created with no Skia `ColorSpace`, which short-circuits the conversion, so tagging the shader's colors with `srgb_linear` was a silent no-op — and subdividing an already-sRGB lerp is a mathematical identity (17x the stops, zero visual effect). `subdivide_gradient_stops` now does the gamma round-trip itself on plain `f32`s, which works whatever color space the destination surface ends up carrying. The wrap above needed one companion fix, found while verifying it rather than reported: geometry is periodic on `spacing` and survives a wrap, but the dot pulse is a `sin` of position and is not. Fed canvas-local coordinates, every dot's radius and alpha stepped at once, every `spacing / speed` seconds. The pulse now reads the unwrapped scroll track, and the test asserting this also asserts that the naive version still steps — so it cannot quietly become vacuous. Tests: 153 + 117 + 6 + 3 + 5 + 215 + 3 pass on this branch alone. --- .../src/legacy_dispatch.rs | 119 +++- .../rustmotion-core/src/engine/paint_pass.rs | 627 ++++++++++++++++-- .../src/engine/render/background.rs | 374 ++++++++++- 3 files changed, 1018 insertions(+), 102 deletions(-) diff --git a/crates/rustmotion-components/src/legacy_dispatch.rs b/crates/rustmotion-components/src/legacy_dispatch.rs index 3b0b023..773bbb8 100644 --- a/crates/rustmotion-components/src/legacy_dispatch.rs +++ b/crates/rustmotion-components/src/legacy_dispatch.rs @@ -119,8 +119,41 @@ impl<'a> PaintDispatcher for LegacyPaintDispatcher<'a> { return; }; + // The `Painter` contract (traits/painter.rs, rules/paint-context.md) + // promises the canvas is already translated to the CONTENT-box + // origin, with `layout` describing the content box — padding + // reserved by taffy is consumed here, not left for the painter to + // rediscover. `Codeblock` is a deliberate, documented exception: it + // reads `style.padding` itself (`codeblock/render.rs` computes + // `code_x = x + pad_left + gutter_width` from the layout origin it + // receives) and paints its own background/border directly from the + // BORDER-box origin. Honoring the general contract for it too would + // double-apply padding — content shifted twice, background rect + // shrunk incorrectly — so it keeps receiving the untranslated + // border-box origin and dimensions, exactly as before this fix. + let is_self_padding = matches!(child.component, Component::Codeblock(_)); + canvas.save(); - canvas.translate((layout.x, layout.y)); + let local = if is_self_padding { + canvas.translate((layout.x, layout.y)); + BoxLayout { + x: 0.0, + y: 0.0, + width: layout.width, + height: layout.height, + ..Default::default() + } + } else { + let (cx, cy, cw, ch) = layout.content_box(); + canvas.translate((cx, cy)); + BoxLayout { + x: 0.0, + y: 0.0, + width: cw, + height: ch, + ..Default::default() + } + }; let paint_ctx = PaintCtx { time: local_time, @@ -131,13 +164,6 @@ impl<'a> PaintDispatcher for LegacyPaintDispatcher<'a> { video_height: frame.video_height, stagger_offset: stagger_delay, }; - let local = BoxLayout { - x: 0.0, - y: 0.0, - width: layout.width, - height: layout.height, - ..Default::default() - }; painter.paint_content(canvas, &local, &props, &paint_ctx); canvas.restore(); @@ -193,6 +219,83 @@ mod tests { } } + #[test] + fn leaf_painter_content_is_inset_by_padding() { + // A 100x80 red shape at (0,0) with `padding: 20`. The Painter + // contract (traits/painter.rs, rules/paint-context.md) promises the + // canvas is already translated to the CONTENT-box origin — so the + // shape's own fill (which just paints (0,0)..(layout.width, + // layout.height)) should only cover the 60x40 content box (20,20) + // to (80,60), leaving the 20px padding ring showing the (empty/ + // background) canvas underneath. Bug: the dispatcher translated to + // the BORDER-box origin and handed the painter the full border-box + // dimensions, so the fill ignored padding entirely and covered the + // whole (0,0)-(100,80) box. + use rustmotion_core::css::style::Edges; + + let mut scene = vec![shape_child(100.0, 80.0, 0.0, 0.0)]; + if let Component::Shape(s) = &mut scene[0].component { + s.style.padding = Some(Edges::Uniform(CLP::Px(20.0))); + } + let built = build_scene(&scene, (200.0, 200.0)); + let layout = run_layout(&built.root, (200.0, 200.0), &ConversionContext::default()); + + let mut surface = + skia_safe::surfaces::raster_n32_premul((200, 200)).expect("raster surface"); + let canvas = surface.canvas(); + let dispatcher = LegacyPaintDispatcher::new(&built.components); + let frame = PaintFrame { + time: 0.0, + frame_index: 0, + fps: 30, + video_width: 200, + video_height: 200, + scene_duration: 1.0, + camera: None, + }; + rustmotion_core::engine::paint_pass::paint_tree( + canvas, + &built.root, + &layout, + &frame, + &dispatcher, + ); + + let snapshot = surface.image_snapshot(); + let info = skia_safe::ImageInfo::new( + (1, 1), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + let read = |x: i32, y: i32| -> [u8; 4] { + let mut buf = [0u8; 4]; + assert!(snapshot.read_pixels( + &info, + &mut buf, + 4, + skia_safe::IPoint::new(x, y), + skia_safe::image::CachingHint::Disallow, + )); + buf + }; + + // Inside the padding ring (5,5): must NOT be red after the fix. + let padding_zone = read(5, 5); + assert!( + !(padding_zone[0] > 200 && padding_zone[1] < 50 && padding_zone[2] < 50), + "padding ring must not be painted by the leaf's own fill, got {:?}", + padding_zone + ); + // Deep inside the content box (50,40): must be red either way. + let content_zone = read(50, 40); + assert!( + content_zone[0] > 200 && content_zone[1] < 50 && content_zone[2] < 50, + "content box must still be painted red, got {:?}", + content_zone + ); + } + #[test] fn dispatch_runs_paint_content_on_leaf() { let scene = vec![shape_child(50.0, 30.0, 10.0, 20.0)]; diff --git a/crates/rustmotion-core/src/engine/paint_pass.rs b/crates/rustmotion-core/src/engine/paint_pass.rs index 3b129e0..f9d454e 100644 --- a/crates/rustmotion-core/src/engine/paint_pass.rs +++ b/crates/rustmotion-core/src/engine/paint_pass.rs @@ -220,9 +220,24 @@ fn paint_node(canvas: &Canvas, node: &BoxNode, ctx: &PaintContext, tree_depth: u viewport_width: ctx.viewport_size.0, viewport_height: ctx.viewport_size.1, parent_size: box_layout.width.max(box_layout.height), - font_size: 16.0, + font_size: node.css.font_size_px_or(16.0), root_font_size: 16.0, }; + // Per-axis contexts for `transform`'s translate percentages: CSS + // resolves a `translate`/`translate3d` x-component percentage against + // the box's own WIDTH and the y-component against its own HEIGHT — never + // `max(width, height)` on both axes (that's only correct for square + // boxes). Mirrors what `resolve_origin` already does for + // `transform-origin` below. `z`/`perspective()` keep the general + // (shared) context — CSS has no per-axis convention for them. + let length_ctx_x = LengthContext { + parent_size: box_layout.width, + ..length_ctx + }; + let length_ctx_y = LengthContext { + parent_size: box_layout.height, + ..length_ctx + }; canvas.save(); @@ -259,13 +274,18 @@ fn paint_node(canvas: &Canvas, node: &BoxNode, ctx: &PaintContext, tree_depth: u .perspective .as_ref() .map(|l| l.resolve(&length_ctx).max(1.0)); + let axes = TransformAxes { + x: length_ctx_x, + y: length_ctx_y, + general: length_ctx, + }; apply_transform( canvas, transform_list, perspective_d, transform_pivot, perspective_pivot, - &length_ctx, + &axes, ); } @@ -293,8 +313,50 @@ fn paint_node(canvas: &Canvas, node: &BoxNode, ctx: &PaintContext, tree_depth: u }); } - // 3. opacity / filter layer — one shared layer carries both the group - // alpha and the CSS `filter` chain (applies to the node and its subtree). + // 3. backdrop-filter: filter what is *already painted behind this node* + // (earlier siblings, ancestor backgrounds), clipped to its own (rounded) + // border-box — the glassmorphism pattern. This must run BEFORE this + // node's own opacity/filter layer (step 4) opens: if it ran after (as it + // used to), the backdrop's `SaveLayerRec::backdrop()` would sample the + // freshly-opened, still-empty opacity layer instead of the real scene + // beneath it, making the blur a total no-op the instant `opacity < 1.0` + // or a `filter` is also present on the same node — exactly the + // glassmorphism + fade_in combination rules/glassmorphism.md recommends. + // Self-contained bracket (save/clip/layer/restore/restore): the panel is + // baked directly onto the canvas below, so this node's own opacity later + // fades its own background/border/content on top of it without + // re-fading the panel itself (avoiding a second, unrelated ordering + // hazard: a shared clip+layer would also have to stay open across + // background/border painting, reintroducing the overflow/shadow bug + // fixed below for those steps too). + if let Some(filters) = node.css.backdrop_filter.as_deref() { + if let Some(backdrop) = filters_to_image_filter(filters, &length_ctx) { + let radius = node + .css + .border_radius + .as_ref() + .map(|r| resolve_border_radius(r, box_layout, &length_ctx)) + .unwrap_or([0.0; 4]); + canvas.save(); + canvas.clip_rrect(border_rrect(box_layout, radius), ClipOp::Intersect, true); + let rec = SaveLayerRec::default().backdrop(&backdrop); + canvas.save_layer(&rec); + canvas.restore(); + canvas.restore(); + } + } + + // 4. opacity / filter layer — one shared layer carries both the group + // alpha and the CSS `filter` chain (applies to the node and its + // subtree). Bounded to the node's own box (padded by the filter chain's + // blur/drop-shadow bleed so those still bleed past the edge, unclipped): + // an unbounded `SaveLayerRec` sizes the layer against the current clip — + // usually the whole viewport — so every faded/filtered node allocates + // and composites a full-frame layer regardless of how small it is + // (measured on this repo's release binary, 1080x1920/60 frames, 30 small + // `opacity: 0.5` shapes, `--threads 1`: ~42-60s wall time unbounded vs. + // ~0.5s bounded — roughly two orders of magnitude, not a rounding + // error; cost scales with viewport area, not node size). let opacity = node.css.opacity.unwrap_or(1.0).clamp(0.0, 1.0); let content_filter = node .css @@ -309,30 +371,33 @@ fn paint_node(canvas: &Canvas, node: &BoxNode, ctx: &PaintContext, tree_depth: u if let Some(filter) = content_filter { paint.set_image_filter(filter); } - let rec = SaveLayerRec::default().paint(&paint); + let bleed = node + .css + .filter + .as_deref() + .map(|list| filter_bleed(list, &length_ctx)) + .unwrap_or(0.0); + let bounds = Rect::from_xywh( + box_layout.x - bleed, + box_layout.y - bleed, + box_layout.width + bleed * 2.0, + box_layout.height + bleed * 2.0, + ); + let rec = SaveLayerRec::default().paint(&paint).bounds(&bounds); canvas.save_layer(&rec); true } else { false }; - // 4. clip overflow:hidden / clip - let overflow = node.css.overflow.unwrap_or(Overflow::Visible); - if matches!( - overflow, - Overflow::Hidden | Overflow::Clip | Overflow::Scroll | Overflow::Auto - ) { - let radius = node - .css - .border_radius - .as_ref() - .map(|r| resolve_border_radius(r, box_layout, &length_ctx)) - .unwrap_or([0.0; 4]); - let rrect = padding_rrect(box_layout, radius); - canvas.clip_rrect(rrect, ClipOp::Intersect, true); - } - - // 5. outset box-shadow + // 5. outset box-shadow, 6. background, 7. border — the box's own + // decorations. Painted BEFORE any overflow clip (step 8): CSS `overflow` + // clips a box's *descendants*, never the box's own border-box + // decorations (an outset box-shadow exists precisely outside the + // border-box; background/border are already shaped by border-radius on + // their own and gain nothing from an extra clip). They still sit inside + // the opacity/filter layer above so a faded node fades its whole + // appearance uniformly, background included. if let Some(shadows) = node.css.box_shadow.as_ref() { for shadow in shadows { if shadow.inset.unwrap_or(false) { @@ -341,41 +406,40 @@ fn paint_node(canvas: &Canvas, node: &BoxNode, ctx: &PaintContext, tree_depth: u paint_box_shadow(canvas, box_layout, &node.css, shadow, &length_ctx, false); } } - - // 5.5 backdrop-filter: filter what is already painted behind this node, - // clipped to its (rounded) border-box, before its own background goes on - // top — the glassmorphism pattern. - if let Some(filters) = node.css.backdrop_filter.as_deref() { - if let Some(backdrop) = filters_to_image_filter(filters, &length_ctx) { - let radius = node - .css - .border_radius - .as_ref() - .map(|r| resolve_border_radius(r, box_layout, &length_ctx)) - .unwrap_or([0.0; 4]); - canvas.save(); - canvas.clip_rrect(border_rrect(box_layout, radius), ClipOp::Intersect, true); - let rec = SaveLayerRec::default().backdrop(&backdrop); - canvas.save_layer(&rec); - canvas.restore(); - canvas.restore(); - } - } - - // 6. background if let Some(bg) = node.css.background.as_ref() { paint_background(canvas, box_layout, &node.css, bg, &length_ctx); } - - // 7. border — `gradient-border` replaces the standard border when present - // (a box has one border, not two stacked ones). + // `gradient-border` replaces the standard border when present (a box + // has one border, not two stacked ones). if let Some(gb) = node.css.gradient_border.as_ref() { paint_gradient_border(canvas, box_layout, &node.css, gb, &length_ctx); } else if let Some(border) = node.css.border.as_ref() { paint_border(canvas, box_layout, &node.css, border, &length_ctx); } - // 8. component-specific content (Ghost is painted identically to Component; + // 8. clip overflow:hidden / clip — scoped to this node's own content and + // its children only (see step 5-7's comment for why the box's own + // decorations must stay outside this clip). + let overflow = node.css.overflow.unwrap_or(Overflow::Visible); + let opened_overflow_clip = if matches!( + overflow, + Overflow::Hidden | Overflow::Clip | Overflow::Scroll | Overflow::Auto + ) { + let radius = node + .css + .border_radius + .as_ref() + .map(|r| resolve_border_radius(r, box_layout, &length_ctx)) + .unwrap_or([0.0; 4]); + let rrect = padding_rrect(box_layout, radius); + canvas.save(); + canvas.clip_rrect(rrect, ClipOp::Intersect, true); + true + } else { + false + }; + + // 9. component-specific content (Ghost is painted identically to Component; // the only difference is that Ghost is excluded from the hit-map above). let payload_opt = match &node.kind { BoxKind::Component(p) | BoxKind::Ghost(p) => Some(p), @@ -386,13 +450,17 @@ fn paint_node(canvas: &Canvas, node: &BoxNode, ctx: &PaintContext, tree_depth: u .dispatch(canvas, payload.as_ref(), &node.css, box_layout, ctx.frame); } - // 9. children (z-index ordered, then source order) + // 10. children (z-index ordered, then source order) let mut indices: Vec = (0..node.children.len()).collect(); indices.sort_by_key(|&i| node.children[i].css.z_index.unwrap_or(0)); for &i in &indices { paint_node(canvas, &node.children[i], ctx, tree_depth + 1); } + if opened_overflow_clip { + canvas.restore(); + } + // inset shadows (after children so they overlay content) if let Some(shadows) = node.css.box_shadow.as_ref() { for shadow in shadows { @@ -408,6 +476,40 @@ fn paint_node(canvas: &Canvas, node: &BoxNode, ctx: &PaintContext, tree_depth: u canvas.restore(); } +/// Conservative outward bleed (px) a `filter` chain can paint beyond the +/// node's own box — used to size the opacity/filter layer's `SaveLayerRec` +/// bounds generously enough that `blur`/`drop-shadow` never get clipped at +/// the box edge (see the perf fix in step 4 above: an unbounded layer costs +/// ~5.9x render time, but a *too-tight* one would silently clip filter +/// bleed, trading a perf bug for a correctness one). `1.5x` the nominal +/// radius covers the visible falloff of `image_filters::blur`'s Gaussian +/// (sigma = radius/2, and ~3*sigma is the point the kernel is visually +/// negligible). +fn filter_bleed(list: &[crate::css::style::FilterFn], ctx: &LengthContext) -> f32 { + use crate::css::style::FilterFn; + let mut bleed = 0.0f32; + for f in list { + let b = match f { + FilterFn::Blur { radius } => radius.resolve(ctx).max(0.0) * 1.5, + FilterFn::DropShadow { + offset_x, + offset_y, + blur, + .. + } => { + let blur_bleed = blur + .as_ref() + .map(|b| b.resolve(ctx).max(0.0) * 1.5) + .unwrap_or(0.0); + offset_x.resolve(ctx).abs().max(offset_y.resolve(ctx).abs()) + blur_bleed + } + _ => 0.0, + }; + bleed = bleed.max(b); + } + bleed +} + // ---- CSS filters ---- /// Build a Skia `ImageFilter` chain from a CSS `filter`/`backdrop-filter` @@ -684,6 +786,19 @@ fn has_3d_transform(list: &[TransformFn]) -> bool { }) } +/// Per-axis length-resolution contexts for `transform`. CSS resolves a +/// `translate`/`translate3d` percentage's x-component against the box's own +/// width and its y-component against its own height — never `max(width, +/// height)` on both axes (see `apply_transform`/`transform_to_m44`). +/// `z`/`perspective()` have no established per-axis CSS convention, so they +/// keep the general (pre-existing) context. +#[derive(Clone, Copy)] +struct TransformAxes { + x: LengthContext, + y: LengthContext, + general: LengthContext, +} + /// Apply CSS transform + perspective to the canvas. /// /// # Parameters @@ -700,7 +815,7 @@ fn apply_transform( perspective_d: Option, transform_pivot: (f32, f32), perspective_pivot: (f32, f32), - ctx: &LengthContext, + axes: &TransformAxes, ) { // Detect whether perspective and transform pivots differ. let pivots_equal = (transform_pivot.0 - perspective_pivot.0).abs() < 0.001 @@ -713,16 +828,16 @@ fn apply_transform( for tr in list { match tr { TransformFn::Translate { x, y } => { - canvas.translate(Point::new(x.resolve(ctx), y.resolve(ctx))); + canvas.translate(Point::new(x.resolve(&axes.x), y.resolve(&axes.y))); } TransformFn::TranslateX { x } => { - canvas.translate(Point::new(x.resolve(ctx), 0.0)); + canvas.translate(Point::new(x.resolve(&axes.x), 0.0)); } TransformFn::TranslateY { y } => { - canvas.translate(Point::new(0.0, y.resolve(ctx))); + canvas.translate(Point::new(0.0, y.resolve(&axes.y))); } TransformFn::Translate3d { x, y, .. } => { - canvas.translate(Point::new(x.resolve(ctx), y.resolve(ctx))); + canvas.translate(Point::new(x.resolve(&axes.x), y.resolve(&axes.y))); } TransformFn::Scale { x, y } => { canvas.scale((*x, *y)); @@ -765,7 +880,7 @@ fn apply_transform( m.pre_concat(&css_perspective_m44(d)); } for tr in list { - m.pre_concat(&transform_to_m44(tr, ctx)); + m.pre_concat(&transform_to_m44(tr, axes)); } m.pre_concat(&M44::translate(-pivot.0, -pivot.1, 0.0)); canvas.concat_44(&m); @@ -789,7 +904,7 @@ fn apply_transform( // Inner transform bracket (transform-origin). m.pre_concat(&M44::translate(tp.0, tp.1, 0.0)); for tr in list { - m.pre_concat(&transform_to_m44(tr, ctx)); + m.pre_concat(&transform_to_m44(tr, axes)); } m.pre_concat(&M44::translate(-tp.0, -tp.1, 0.0)); @@ -820,15 +935,19 @@ fn css_perspective_m44(d: f32) -> M44 { ]) } -fn transform_to_m44(tr: &TransformFn, ctx: &LengthContext) -> M44 { +fn transform_to_m44(tr: &TransformFn, axes: &TransformAxes) -> M44 { match tr { - TransformFn::Translate { x, y } => M44::translate(x.resolve(ctx), y.resolve(ctx), 0.0), - TransformFn::TranslateX { x } => M44::translate(x.resolve(ctx), 0.0, 0.0), - TransformFn::TranslateY { y } => M44::translate(0.0, y.resolve(ctx), 0.0), - TransformFn::TranslateZ { z } => M44::translate(0.0, 0.0, z.resolve(ctx)), - TransformFn::Translate3d { x, y, z } => { - M44::translate(x.resolve(ctx), y.resolve(ctx), z.resolve(ctx)) + TransformFn::Translate { x, y } => { + M44::translate(x.resolve(&axes.x), y.resolve(&axes.y), 0.0) } + TransformFn::TranslateX { x } => M44::translate(x.resolve(&axes.x), 0.0, 0.0), + TransformFn::TranslateY { y } => M44::translate(0.0, y.resolve(&axes.y), 0.0), + TransformFn::TranslateZ { z } => M44::translate(0.0, 0.0, z.resolve(&axes.general)), + TransformFn::Translate3d { x, y, z } => M44::translate( + x.resolve(&axes.x), + y.resolve(&axes.y), + z.resolve(&axes.general), + ), TransformFn::Scale { x, y } => M44::scale(*x, *y, 1.0), TransformFn::ScaleX { x } => M44::scale(*x, 1.0, 1.0), TransformFn::ScaleY { y } => M44::scale(1.0, *y, 1.0), @@ -896,7 +1015,9 @@ fn transform_to_m44(tr: &TransformFn, ctx: &LengthContext) -> M44 { 0.0, 1.0, ]), - TransformFn::Perspective { length } => css_perspective_m44(length.resolve(ctx).max(1.0)), + TransformFn::Perspective { length } => { + css_perspective_m44(length.resolve(&axes.general).max(1.0)) + } TransformFn::Matrix { values: v } => M44::row_major(&[ v[0], v[2], 0.0, v[4], v[1], v[3], 0.0, v[5], 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 0.0, 1.0, ]), @@ -1519,6 +1640,113 @@ mod hit_tests { ); } + #[test] + fn backdrop_filter_survives_sibling_opacity_below_one() { + // Same scene as `backdrop_filter_blurs_content_behind`, but the panel + // also carries `opacity: 0.99` — a visually-imperceptible change that + // must NOT disable the blur. Bug: the opacity/filter SaveLayerRec + // (paint_pass step 3) used to open BEFORE the backdrop-filter's own + // save_layer(backdrop) (old step 5.5), so the backdrop sampled the + // freshly-opened, still-empty opacity layer instead of the real + // scene beneath it — a total no-op. `opacity` alone (no + // `backdrop_filter`) is not the trigger; only nodes that combine + // both are affected, which is exactly the documented glassmorphism + // template (glassmorphism.md pairs `backdrop-filter` with a + // `fade_in`/`fade_in_up` entrance animation that drives `opacity`). + use crate::css::style::{Background, Color as CssColor, FilterFn}; + use crate::css::units::Length; + + let black_top = BoxNode { + id: 0, + kind: BoxKind::Container, + css: CssStyle { + position: Some(Position::Absolute), + left: Some(CLP::Px(0.0)), + top: Some(CLP::Px(0.0)), + width: Some(CSize::Length(CLP::Px(200.0))), + height: Some(CSize::Length(CLP::Px(100.0))), + background: Some(Background::Color(CssColor::String("#000000".into()))), + ..Default::default() + }, + children: vec![], + intrinsic: None, + source_path: None, + window: None, + }; + let panel = BoxNode { + id: 0, + kind: BoxKind::Container, + css: CssStyle { + position: Some(Position::Absolute), + left: Some(CLP::Px(50.0)), + top: Some(CLP::Px(50.0)), + width: Some(CSize::Length(CLP::Px(100.0))), + height: Some(CSize::Length(CLP::Px(100.0))), + backdrop_filter: Some(vec![FilterFn::Blur { + radius: Length::Px(10.0), + }]), + opacity: Some(0.99), + ..Default::default() + }, + children: vec![], + intrinsic: None, + source_path: None, + window: None, + }; + let mut root = BoxNode { + id: 0, + kind: BoxKind::Container, + css: CssStyle { + display: Some(Display::Flex), + width: Some(CSize::Length(CLP::Px(200.0))), + height: Some(CSize::Length(CLP::Px(200.0))), + background: Some(Background::Color(CssColor::String("#ffffff".into()))), + ..Default::default() + }, + children: vec![black_top, panel], + intrinsic: None, + source_path: None, + window: None, + }; + root.assign_ids(0); + + let layout = run_layout(&root, (200.0, 200.0), &ConversionContext::default()); + let mut surface = skia_safe::surfaces::raster_n32_premul((200, 200)).unwrap(); + paint_tree( + surface.canvas(), + &root, + &layout, + &test_frame(200, 200), + &NoopDispatcher, + ); + + let info = skia_safe::ImageInfo::new( + (200, 200), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Unpremul, + None, + ); + let mut buf = vec![0u8; 200 * 200 * 4]; + assert!(surface.read_pixels(&info, &mut buf, 200 * 4, (0, 0))); + let red = |x: usize, y: usize| buf[(y * 200 + x) * 4] as i32; + + // Outside the panel: hard edge preserved. + assert!(red(10, 97) < 10, "outside/above must stay black"); + assert!(red(10, 103) > 245, "outside/below must stay white"); + // Inside the panel: boundary must still smear into greys — the bug + // makes this a hard 0/255 edge identical to the outside columns. + let above = red(100, 97); + let below = red(100, 103); + assert!( + above > 30, + "backdrop not blurred above boundary with opacity:0.99 (r={above})" + ); + assert!( + below < 225, + "backdrop not blurred below boundary with opacity:0.99 (r={below})" + ); + } + #[test] fn hitmap_reflects_node_transform() { use crate::css::style::TransformFn; @@ -1970,6 +2198,54 @@ mod transform_origin_tests { "expected some red pixels with distinct origins" ); } + + // ---- Test 8: translate percentages resolve per-axis, not max(w,h) ---- + + #[test] + fn translate_percent_resolves_against_own_axis_not_max_dimension() { + // A 200x100 red box at (0,0), `transform: translate(50%, 50%)`. + // CSS resolves a translate-x percentage against the box's own WIDTH + // and translate-y against its own HEIGHT — never `max(width, + // height)` on both axes (only correct for square boxes). Expected: + // x shifts by 100 (50% of 200) -> [100,299]; y shifts by 50 (50% of + // 100) -> [50,149]. The bug instead resolved y against max(200,100) + // = 200, doubling the vertical shift to +100 -> [100,199]. + let mut n = red_box(Position::Absolute, 0.0, 0.0, 200.0, 100.0); + n.css.transform = Some(vec![TransformFn::Translate { + x: CLP::String("50%".into()), + y: CLP::String("50%".into()), + }]); + let mut root = root_node(400.0, 400.0, vec![n]); + let buf = render_pixels(&mut root, 400, 400); + + let mut min_x = u32::MAX; + let mut max_x = 0u32; + let mut min_y = u32::MAX; + let mut max_y = 0u32; + for y in 0..400u32 { + for x in 0..400u32 { + let i = ((y * 400 + x) * 4) as usize; + if buf[i] > 200 && buf[i + 1] < 50 && buf[i + 2] < 50 { + min_x = min_x.min(x); + max_x = max_x.max(x); + min_y = min_y.min(y); + max_y = max_y.max(y); + } + } + } + assert_ne!(min_x, u32::MAX, "expected some red pixels"); + + assert!((min_x as i32 - 100).abs() <= 2, "min_x={min_x}"); + assert!((max_x as i32 - 299).abs() <= 2, "max_x={max_x}"); + assert!( + (min_y as i32 - 50).abs() <= 2, + "min_y={min_y} (expected ~50; the max(w,h) bug would give ~100)" + ); + assert!( + (max_y as i32 - 149).abs() <= 2, + "max_y={max_y} (expected ~149; the max(w,h) bug would give ~199)" + ); + } } #[cfg(test)] @@ -2395,6 +2671,229 @@ mod glassmorphism_tests { } } +#[cfg(test)] +mod paint_order_tests { + //! TDD tests for two paint-pass audit findings: + //! - overflow:hidden must never clip a node's OWN outset box-shadow + //! (CSS clips descendants, never the box's own decorations). + //! - the opacity/filter SaveLayerRec must be bounded to the node's box + //! (+ filter bleed), not left to size against the ambient clip + //! (usually the whole viewport) — without clipping visible blur. + + use super::*; + + use crate::css::style::{ + Background, BoxShadow, Color as CssColor, CssStyle, Display, FilterFn, FlexDirection, + Overflow, Position, Size as CSize, + }; + use crate::css::taffy_bridge::ConversionContext; + use crate::css::units::{Length, LengthPercentage as CLP}; + use crate::engine::box_tree::{BoxKind, BoxNode}; + use crate::engine::layout_pass::run_layout; + + fn test_frame(w: u32, h: u32) -> PaintFrame { + PaintFrame { + time: 0.0, + frame_index: 0, + fps: 30, + video_width: w, + video_height: h, + scene_duration: 1.0, + camera: None, + } + } + + fn render_pixels(root: &mut BoxNode, w: u32, h: u32) -> Vec { + root.assign_ids(0); + let layout = run_layout(root, (w as f32, h as f32), &ConversionContext::default()); + let mut surface = skia_safe::surfaces::raster_n32_premul((w as i32, h as i32)).unwrap(); + paint_tree( + surface.canvas(), + root, + &layout, + &test_frame(w, h), + &NoopDispatcher, + ); + let info = skia_safe::ImageInfo::new( + (w as i32, h as i32), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Unpremul, + None, + ); + let mut buf = vec![0u8; (w * h * 4) as usize]; + surface.read_pixels(&info, &mut buf, (w * 4) as usize, (0, 0)); + buf + } + + fn root_node(w: f32, h: f32, background: &str, children: Vec) -> BoxNode { + BoxNode { + id: 0, + kind: BoxKind::Container, + css: CssStyle { + display: Some(Display::Flex), + flex_direction: Some(FlexDirection::Column), + width: Some(CSize::Length(CLP::Px(w))), + height: Some(CSize::Length(CLP::Px(h))), + background: Some(Background::Color(CssColor::String(background.to_string()))), + ..Default::default() + }, + children, + intrinsic: None, + source_path: None, + window: None, + } + } + + /// Count of "probe" red pixels (spread-only, blur:0, so a hard-edged + /// halo) in a rectangular region — used to compare before/after pixel + /// counts for the paint-order fix. + fn count_red_in(buf: &[u8], w: u32, x0: u32, y0: u32, x1: u32, y1: u32) -> usize { + let mut n = 0; + for y in y0..y1 { + for x in x0..x1 { + let i = ((y * w + x) * 4) as usize; + if buf[i] > 200 && buf[i + 1] < 50 && buf[i + 2] < 50 { + n += 1; + } + } + } + n + } + + fn card_with_shadow(overflow_hidden: bool) -> BoxNode { + let mut css = CssStyle { + position: Some(Position::Absolute), + left: Some(CLP::Px(50.0)), + top: Some(CLP::Px(50.0)), + width: Some(CSize::Length(CLP::Px(100.0))), + height: Some(CSize::Length(CLP::Px(100.0))), + background: Some(Background::Color(CssColor::String("#ffffff".into()))), + box_shadow: Some(vec![BoxShadow { + offset_x: Length::Px(0.0), + offset_y: Length::Px(0.0), + blur: None, + spread: Some(Length::Px(20.0)), + color: Some(CssColor::String("#ff0000".into())), + inset: None, + }]), + ..Default::default() + }; + if overflow_hidden { + css.overflow = Some(Overflow::Hidden); + } + BoxNode { + id: 0, + kind: BoxKind::Container, + css, + children: vec![], + intrinsic: None, + source_path: None, + window: None, + } + } + + #[test] + fn overflow_hidden_does_not_clip_own_outset_box_shadow() { + // 100x100 white card at (50,50) on a 200x200 black canvas, outset + // box-shadow (red, spread 20, blur 0 -> hard-edged halo rect from + // (30,30) to (170,170)). Probe points (100,45) and (100,155) sit in + // the halo band above/below the card, outside its own border-box. + let without = { + let mut root = root_node(200.0, 200.0, "#000000", vec![card_with_shadow(false)]); + render_pixels(&mut root, 200, 200) + }; + let with_hidden = { + let mut root = root_node(200.0, 200.0, "#000000", vec![card_with_shadow(true)]); + render_pixels(&mut root, 200, 200) + }; + + let probe = |buf: &[u8], x: usize, y: usize| -> (u8, u8, u8) { + let i = (y * 200 + x) * 4; + (buf[i], buf[i + 1], buf[i + 2]) + }; + + let above_plain = probe(&without, 100, 45); + let below_plain = probe(&without, 100, 155); + assert!( + above_plain.0 > 200 && above_plain.1 < 50, + "sanity: shadow halo must be visible without overflow, got {above_plain:?}" + ); + assert!( + below_plain.0 > 200 && below_plain.1 < 50, + "sanity: shadow halo must be visible without overflow, got {below_plain:?}" + ); + + let above_hidden = probe(&with_hidden, 100, 45); + let below_hidden = probe(&with_hidden, 100, 155); + assert!( + above_hidden.0 > 200 && above_hidden.1 < 50, + "overflow:hidden must not erase the node's own outset shadow, got {above_hidden:?}" + ); + assert!( + below_hidden.0 > 200 && below_hidden.1 < 50, + "overflow:hidden must not erase the node's own outset shadow, got {below_hidden:?}" + ); + + // Probe-pixel count over the full halo band, before/after overflow. + let halo_count_plain = count_red_in(&without, 200, 25, 25, 175, 175); + let halo_count_hidden = count_red_in(&with_hidden, 200, 25, 25, 175, 175); + assert_eq!( + halo_count_plain, halo_count_hidden, + "halo pixel count must be identical with/without overflow:hidden \ + (plain={halo_count_plain}, hidden={halo_count_hidden})" + ); + } + + #[test] + fn filter_layer_bounds_do_not_clip_blur_bleed() { + // A 60x60 opaque red square with `opacity: 0.999` (forces the + // SaveLayerRec open) AND `filter: blur(24px)` on a 300x300 black + // canvas. Bounding the layer to the node's box (issue #4 fix) must + // still leave room for the blur to bleed outward — if the bounds + // were the bare box rect, Skia would hard-clip the blurred fringe + // at the box edge, and the region just outside the box would stay + // pure black instead of picking up a soft red glow. + let n = BoxNode { + id: 0, + kind: BoxKind::Container, + css: CssStyle { + position: Some(Position::Absolute), + left: Some(CLP::Px(120.0)), + top: Some(CLP::Px(120.0)), + width: Some(CSize::Length(CLP::Px(60.0))), + height: Some(CSize::Length(CLP::Px(60.0))), + background: Some(Background::Color(CssColor::String("#ff0000".into()))), + opacity: Some(0.999), + filter: Some(vec![FilterFn::Blur { + radius: Length::Px(24.0), + }]), + ..Default::default() + }, + children: vec![], + intrinsic: None, + source_path: None, + window: None, + }; + let mut root = root_node(300.0, 300.0, "#000000", vec![n]); + let buf = render_pixels(&mut root, 300, 300); + + let probe = |x: usize, y: usize| -> u8 { + let i = (y * 300 + x) * 4; + buf[i] + }; + // 8px outside the left edge of the box (box left edge = x=120), + // vertically centered (y=150): must show blur bleed (red > black). + let bled = probe(112, 150); + assert!( + bled > 15, + "blur must bleed past the box edge under bounded SaveLayerRec, got r={bled}" + ); + // Far outside any plausible bleed radius: must stay black. + let far = probe(20, 20); + assert_eq!(far, 0, "far corner must stay untouched, got r={far}"); + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/rustmotion/src/engine/render/background.rs b/crates/rustmotion/src/engine/render/background.rs index c507947..6492858 100644 --- a/crates/rustmotion/src/engine/render/background.rs +++ b/crates/rustmotion/src/engine/render/background.rs @@ -17,6 +17,14 @@ pub(super) fn draw_animated_background( // Compute scroll offset for tiled presets (gradient_shift handles rotation internally) let (scroll_x, scroll_y) = compute_scroll_offset(bg, time); + // Whole tile periods the wrap removed. The geometry doesn't care (the + // pattern is periodic on `spacing`) but `draw_bg_grid_dots`'s pulse is + // a function of position, so without adding these back its phase would + // jump by `spacing * 0.01` radians every time the offset wraps — a + // visible, periodic pop in every dot's radius and alpha at once. + let (raw_x, raw_y) = raw_scroll_offset(bg, time); + let phase_origin = (raw_x - scroll_x, raw_y - scroll_y); + canvas.save(); canvas.translate((bg.x + scroll_x, bg.y + scroll_y)); @@ -30,7 +38,9 @@ pub(super) fn draw_animated_background( width, height, ), - BackgroundPreset::GridDots(cfg) => draw_bg_grid_dots(canvas, cfg, time, width, height), + BackgroundPreset::GridDots(cfg) => { + draw_bg_grid_dots(canvas, cfg, time, width, height, phase_origin) + } BackgroundPreset::ConcentricCircles(cfg) => { draw_bg_concentric_circles(canvas, cfg, bg.speed, time, width, height) } @@ -64,15 +74,7 @@ pub(super) fn draw_world_bg_with_parallax( } _ => { // Grid-based backgrounds: modulo offset for seamless tiling. - let spacing = match &bg.preset { - BackgroundPreset::GridDots(cfg) => cfg.spacing.max(20.0), - BackgroundPreset::ConcentricCircles(cfg) => cfg.spacing.max(20.0), - BackgroundPreset::Heropattern(cfg) => { - let def = crate::engine::heropatterns::find_pattern(&cfg.pattern); - def.map(|d| d.width * cfg.scale).unwrap_or(60.0).max(20.0) - } - _ => 60.0_f32.max(20.0), - }; + let spacing = tile_spacing(&bg.preset); let offset_x = -(cam_x % spacing); let offset_y = -(cam_y % spacing); canvas.save(); @@ -115,10 +117,11 @@ fn draw_bg_gradient_shift( let angle = (sign * speed * time) % 360.0; let rad = angle.to_radians(); - // Interpolate in linear color space to reduce banding on dark gradients - let linear_cs = skia_safe::ColorSpace::new_srgb_linear(); - - // Subdivide color stops (16 intermediate steps between each pair) for smoother gradients + // Subdivide color stops (16 intermediate steps between each pair), + // interpolating in *linear light* and re-encoding to sRGB per generated + // stop — see `subdivide_gradient_stops` for why this can't be delegated + // to a Skia `ColorSpace` tag on the shader (the render surfaces carry no + // color space, so any such tag is a silent no-op). let (colors, positions) = subdivide_gradient_stops(&base_colors, 16); let shader = match cfg.gradient_type { @@ -130,7 +133,7 @@ fn draw_bg_gradient_shift( let end = Point::new(cx + rad.cos() * half_diag, cy + rad.sin() * half_diag); skia_safe::shader::Shader::linear_gradient( (start, end), - GradientShaderColors::ColorsInSpace(&colors, Some(linear_cs)), + GradientShaderColors::ColorsInSpace(&colors, None), Some(&positions[..]), skia_safe::TileMode::Clamp, None, @@ -143,7 +146,7 @@ fn draw_bg_gradient_shift( skia_safe::shader::Shader::radial_gradient( center, radius, - GradientShaderColors::ColorsInSpace(&colors, Some(linear_cs)), + GradientShaderColors::ColorsInSpace(&colors, None), Some(&positions[..]), skia_safe::TileMode::Clamp, None, @@ -160,8 +163,25 @@ fn draw_bg_gradient_shift( } } -/// Subdivide gradient color stops by inserting intermediate interpolated colors. -/// Returns (colors, positions) with `subdivisions` extra stops between each original pair. +/// Subdivide gradient color stops by inserting intermediate interpolated +/// colors. Returns (colors, positions) with `subdivisions` extra stops +/// between each original pair. +/// +/// RGB is interpolated in **linear light** (decoded from sRGB, lerped, +/// re-encoded to sRGB per generated stop) — the actual fix for +/// rules/gradient-quality.md's "linear color space interpolation" claim. +/// The two mitigations used to rely on Skia: tagging the shader's colors +/// with `ColorSpace::new_srgb_linear()` and subdividing so Skia's own +/// per-pixel lerp had more (supposedly linear-space) stops to work with. +/// Both were silent no-ops: the render surfaces are created with no color +/// space (`ImageInfo::new(..., None)`), which short-circuits any +/// color-space conversion Skia would otherwise do — so the colors stayed +/// gamma-encoded sRGB the whole time, and subdividing an already-sRGB lerp +/// is a mathematical identity (17x more stops, zero visual effect). Doing +/// the gamma conversion here, on plain `f32`s, works regardless of what +/// color space (if any) the destination surface ends up tagged with later. +/// +/// Alpha is NOT gamma-encoded and keeps a plain linear lerp. pub(super) fn subdivide_gradient_stops( colors: &[skia_safe::Color4f], subdivisions: u32, @@ -182,22 +202,56 @@ pub(super) fn subdivide_gradient_stops( for s in 0..steps { let t = s as f32 / steps as f32; let global_t = (i as f32 + t) / seg; - out_colors.push(skia_safe::Color4f { - r: c0.r + (c1.r - c0.r) * t, - g: c0.g + (c1.g - c0.g) * t, - b: c0.b + (c1.b - c0.b) * t, - a: c0.a + (c1.a - c0.a) * t, - }); + let color = if t == 0.0 { + // Exact copy at the segment start — no conversion round-trip + // drift on stops that already existed pre-subdivision. + *c0 + } else { + skia_safe::Color4f { + r: lerp_srgb_channel(c0.r, c1.r, t), + g: lerp_srgb_channel(c0.g, c1.g, t), + b: lerp_srgb_channel(c0.b, c1.b, t), + a: c0.a + (c1.a - c0.a) * t, + } + }; + out_colors.push(color); out_pos.push(global_t); } } - // Last color + // Last color — exact copy, same reasoning as the `t == 0.0` case above. out_colors.push(colors[n - 1]); out_pos.push(1.0); (out_colors, out_pos) } +/// Lerp one sRGB-encoded channel (0..1) by decoding both endpoints to linear +/// light, interpolating there, and re-encoding back to sRGB. +fn lerp_srgb_channel(a: f32, b: f32, t: f32) -> f32 { + let linear = srgb_to_linear(a) + (srgb_to_linear(b) - srgb_to_linear(a)) * t; + linear_to_srgb(linear) +} + +/// sRGB EOTF (decode): gamma-encoded 0..1 -> linear light 0..1. +fn srgb_to_linear(c: f32) -> f32 { + let c = c.clamp(0.0, 1.0); + if c <= 0.04045 { + c / 12.92 + } else { + ((c + 0.055) / 1.055).powf(2.4) + } +} + +/// sRGB OETF (encode): linear light 0..1 -> gamma-encoded 0..1. +fn linear_to_srgb(c: f32) -> f32 { + let c = c.clamp(0.0, 1.0); + if c <= 0.0031308 { + c * 12.92 + } else { + 1.055 * c.powf(1.0 / 2.4) - 0.055 + } +} + /// Soft colored glow zones (halo preset). fn draw_bg_halo(canvas: &Canvas, cfg: &HaloConfig, speed: f32, time: f32, width: f32, height: f32) { for (i, zone) in cfg.zones.iter().enumerate() { @@ -274,21 +328,48 @@ fn draw_bg_concentric_circles( } } +/// Pulse factor (radius multiplier and alpha basis) of the dot drawn at +/// canvas-local `(x, y)`. +/// +/// `phase_origin` is the whole number of tile periods `compute_scroll_offset` +/// wrapped away, and is subtracted so the argument stays the dot's position +/// on the *unwrapped* scroll track. Geometry is periodic on `spacing` and so +/// survives the wrap unchanged; this `sin` is not, and would otherwise step +/// by `spacing * 0.01` rad at every wrap. +fn dot_pulse(x: f32, y: f32, time: f32, phase_origin: (f32, f32)) -> f32 { + let wx = x - phase_origin.0; + let wy = y - phase_origin.1; + (wx * 0.01 + wy * 0.01 + time * 2.0).sin() * 0.3 + 0.7 +} + /// Animated dot grid pattern. -fn draw_bg_grid_dots(canvas: &Canvas, cfg: &GridDotsConfig, time: f32, width: f32, height: f32) { +fn draw_bg_grid_dots( + canvas: &Canvas, + cfg: &GridDotsConfig, + time: f32, + width: f32, + height: f32, + phase_origin: (f32, f32), +) { let mut paint = paint_from_hex(&cfg.color); paint.set_anti_alias(true); let spacing = cfg.spacing.max(20.0); let dot_radius = cfg.element_size / 2.0; - // Scroll is now handled by compute_scroll_offset + canvas translate upstream. + // Scroll is handled by compute_scroll_offset + canvas translate + // upstream, which now wraps the offset into `(-spacing, spacing)` (see + // `compute_scroll_offset`) — this loop must overscan symmetrically on + // BOTH axes (one `spacing` of margin on every side) to still cover the + // full viewport for any offset in that range. The x-loop used to start + // at 0 with no left margin (asymmetric vs. the y-loop below), so any + // positive scroll left a growing blank band on the left edge. let mut y = -spacing; while y < height + spacing { - let mut x = 0.0_f32; + let mut x = -spacing; while x < width + spacing { // Pulse: subtle size variation based on position + time - let phase = (x * 0.01 + y * 0.01 + time * 2.0).sin() * 0.3 + 0.7; + let phase = dot_pulse(x, y, time, phase_origin); let r = dot_radius * phase; paint.set_alpha_f(phase * 0.4); canvas.draw_circle((x, y), r, &paint); @@ -520,8 +601,50 @@ pub(super) fn interpolate_animated_bg( } } -/// Compute the scroll offset for tiled backgrounds based on direction + speed. +/// Tile period (px) a preset's own draw loop repeats on — the amount by +/// which a scroll offset can be wrapped without changing the rendered +/// pattern. Shared by `compute_scroll_offset` (below) and +/// `draw_world_bg_with_parallax`'s camera-pan modulo so the two never +/// diverge on what "one period" means for a given preset. +fn tile_spacing(preset: &BackgroundPreset) -> f32 { + match preset { + BackgroundPreset::GridDots(cfg) => cfg.spacing.max(20.0), + BackgroundPreset::ConcentricCircles(cfg) => cfg.spacing.max(20.0), + BackgroundPreset::Heropattern(cfg) => { + let def = crate::engine::heropatterns::find_pattern(&cfg.pattern); + def.map(|d| d.width * cfg.scale).unwrap_or(60.0).max(20.0) + } + _ => 60.0_f32.max(20.0), + } +} + +/// Compute the scroll offset for tiled backgrounds based on direction + +/// speed, wrapped into `(-spacing, spacing)` so it never grows unbounded. +/// +/// Bug this fixes: the offset used to grow linearly with `time` forever. +/// The tiled draw loops (`draw_bg_grid_dots`, `draw_bg_heropattern`) only +/// ever overscan by one `spacing`/`margin` around the viewport — with the +/// canvas translated by an unbounded offset, the pattern slides off-frame +/// and leaves a growing blank band once the offset exceeds that one-tile +/// margin (see paint.md finding #5). Since every tiled pattern is exactly +/// periodic on `spacing`, translating by any offset congruent mod `spacing` +/// produces byte-identical pixels — Rust's `%` already returns a value with +/// `|result| < spacing` and the same sign as the input, which is exactly +/// the symmetric `(-spacing, spacing)` margin the (now-symmetric, see +/// `draw_bg_grid_dots`) draw loops need. `t=0` (or `speed=0`) stays an exact +/// `(0.0, 0.0)` no-op — `0.0 % spacing == 0.0`. pub(super) fn compute_scroll_offset(bg: &AnimatedBackground, time: f32) -> (f32, f32) { + let (raw_x, raw_y) = raw_scroll_offset(bg, time); + let spacing = tile_spacing(&bg.preset); + (raw_x % spacing, raw_y % spacing) +} + +/// The unwrapped scroll offset — how far the pattern *would* have travelled +/// under the old unbounded scheme. Only `compute_scroll_offset` (for the +/// wrap) and the grid-dot pulse phase (for continuity across a wrap, see +/// `phase_origin` in `draw_animated_background`) need this; nothing should +/// translate a canvas by it. +fn raw_scroll_offset(bg: &AnimatedBackground, time: f32) -> (f32, f32) { let speed = bg.speed; if speed == 0.0 { return (0.0, 0.0); @@ -653,3 +776,194 @@ mod halo_opacity_tests { ); } } + +#[cfg(test)] +mod scroll_offset_wrap_tests { + //! TDD tests for paint.md finding #5: `compute_scroll_offset` must wrap + //! into one tile period instead of growing unbounded, or the tiled + //! background's draw loops (which only ever overscan by one `spacing` + //! around the viewport) leave a growing blank band. + + use super::*; + use crate::schema::GridDotsConfig; + + fn grid_bg(direction: ScrollDirection, speed: f32) -> AnimatedBackground { + AnimatedBackground { + preset: BackgroundPreset::GridDots(GridDotsConfig { + color: "#ffffff".into(), + element_size: 8.0, + spacing: 40.0, + }), + x: 0.0, + y: 0.0, + speed, + direction: Some(direction), + } + } + + #[test] + fn scroll_offset_stays_within_one_tile_period() { + // Repro (paint.md #5): 300x200, grid_dots, speed 60, direction + // right, spacing 40 — at t=3s the raw offset is 180px (4.5 + // spacings), way outside what `draw_bg_grid_dots`'s one-tile + // overscan margin can cover. + let bg = grid_bg(ScrollDirection::Right, 60.0); + for t in [0.0f32, 0.1, 0.5, 1.0, 3.0, 10.0, 37.3] { + let (dx, dy) = compute_scroll_offset(&bg, t); + assert!( + (-40.0..=40.0).contains(&dx), + "t={t}: dx={dx} must stay within one tile period (±spacing=40)" + ); + assert_eq!( + dy, 0.0, + "t={t}: pure horizontal scroll must not drift vertically (dy={dy})" + ); + } + } + + #[test] + fn scroll_offset_at_t0_is_unchanged_zero() { + // Non-regression: t=0 must stay an exact no-op, not jump to a whole + // tile period ahead/behind. + let bg = grid_bg(ScrollDirection::Right, 60.0); + assert_eq!(compute_scroll_offset(&bg, 0.0), (0.0, 0.0)); + } + + #[test] + fn scroll_offset_zero_speed_is_still_a_pure_noop() { + let bg = grid_bg(ScrollDirection::Right, 0.0); + assert_eq!(compute_scroll_offset(&bg, 5.0), (0.0, 0.0)); + } + + /// Regression guard for a side effect of the wrap itself: geometry is + /// periodic on `spacing` so it crosses a wrap unchanged, but the dot + /// pulse is a `sin` of position and is not. Feeding it canvas-local + /// coordinates made every dot's radius and alpha step at once, once per + /// `spacing / speed` seconds. + #[test] + fn dot_pulse_is_continuous_across_a_wrap() { + let bg = grid_bg(ScrollDirection::Right, 60.0); + // spacing 40 / speed 60 => the offset wraps at t = 2/3 s. + let (before, after) = (0.6666_f32, 0.6667_f32); + assert!( + compute_scroll_offset(&bg, before).0 > compute_scroll_offset(&bg, after).0, + "test setup: these two instants must straddle a wrap" + ); + + // Pulse of whichever dot lands on a fixed screen position. + let (sx_screen, sy_screen) = (200.0_f32, 100.0_f32); + let sampled = |t: f32| { + let (sx, sy) = compute_scroll_offset(&bg, t); + let (rx, ry) = raw_scroll_offset(&bg, t); + dot_pulse(sx_screen - sx, sy_screen - sy, t, (rx - sx, ry - sy)) + }; + let delta = (sampled(after) - sampled(before)).abs(); + assert!( + delta < 0.01, + "pulse must not jump across a wrap, got delta {delta}" + ); + + // Witness that this is a real hazard and not a vacuous assertion: + // the same sample without the phase origin does step visibly. + let naive = |t: f32| { + let (sx, sy) = compute_scroll_offset(&bg, t); + dot_pulse(sx_screen - sx, sy_screen - sy, t, (0.0, 0.0)) + }; + assert!( + (naive(after) - naive(before)).abs() > 0.05, + "canvas-local phase should step at a wrap — if it no longer does, \ + this test has stopped proving anything" + ); + } + + /// The pulse must be untouched before the first wrap, so the fix cannot + /// change how any existing scenario's opening seconds look. + #[test] + fn dot_pulse_matches_the_original_formula_with_no_wrap_yet() { + let bg = grid_bg(ScrollDirection::Right, 60.0); + for t in [0.0_f32, 0.1, 0.5] { + let (sx, sy) = compute_scroll_offset(&bg, t); + let (rx, ry) = raw_scroll_offset(&bg, t); + assert_eq!( + (rx - sx, ry - sy), + (0.0, 0.0), + "t={t}: no whole period wrapped away yet" + ); + let expected = (40.0_f32 * 0.01 + 20.0 * 0.01 + t * 2.0).sin() * 0.3 + 0.7; + assert_eq!(dot_pulse(40.0, 20.0, t, (rx - sx, ry - sy)), expected); + } + } + + #[test] + fn scroll_offset_wraps_consistently_for_left_direction_too() { + let bg = grid_bg(ScrollDirection::Left, 60.0); + for t in [0.0f32, 3.0, 10.0] { + let (dx, _dy) = compute_scroll_offset(&bg, t); + assert!( + (-40.0..=40.0).contains(&dx), + "t={t}: dx={dx} must stay within one tile period" + ); + } + } +} + +#[cfg(test)] +mod gradient_linear_space_tests { + //! TDD tests for paint.md finding #7: the "linear color space + //! interpolation" and "subdivided stops" banding mitigations were both + //! inert (the render surfaces carry no Skia `ColorSpace`, so the + //! `ColorsInSpace(..., Some(linear_cs))` tag was a silent no-op, and + //! subdividing an already-sRGB-space lerp is a mathematical identity). + //! Fix: `subdivide_gradient_stops` itself now interpolates in linear + //! light and re-encodes to sRGB per generated stop. + + use super::*; + use skia_safe::Color4f; + + #[test] + fn subdivide_interpolates_in_linear_light_not_srgb_gamma() { + // Black -> white: the true midpoint in *linear light* (0.5) encodes + // back to sRGB as ~188, not the naive sRGB-byte midpoint of 127. + let black = Color4f::new(0.0, 0.0, 0.0, 1.0); + let white = Color4f::new(1.0, 1.0, 1.0, 1.0); + let (colors, positions) = subdivide_gradient_stops(&[black, white], 1); + assert_eq!(positions.len(), 3, "1 subdivision -> stops at 0, 0.5, 1.0"); + assert_eq!(positions[1], 0.5); + let mid_255 = (colors[1].r * 255.0).round() as i32; + assert!( + (mid_255 - 188).abs() <= 3, + "midpoint should be ~188 (linear-light average re-encoded to sRGB), got {mid_255}" + ); + } + + #[test] + fn subdivide_endpoints_are_exact() { + let a = Color4f::new(0.2, 0.4, 0.6, 1.0); + let b = Color4f::new(0.8, 0.1, 0.9, 1.0); + let (colors, positions) = subdivide_gradient_stops(&[a, b], 16); + assert_eq!(positions[0], 0.0); + assert_eq!(*positions.last().unwrap(), 1.0); + let first = colors[0]; + let last = *colors.last().unwrap(); + assert!((first.r - a.r).abs() < 1e-4, "first.r={}", first.r); + assert!((first.g - a.g).abs() < 1e-4, "first.g={}", first.g); + assert!((first.b - a.b).abs() < 1e-4, "first.b={}", first.b); + assert!((last.r - b.r).abs() < 1e-4, "last.r={}", last.r); + assert!((last.g - b.g).abs() < 1e-4, "last.g={}", last.g); + assert!((last.b - b.b).abs() < 1e-4, "last.b={}", last.b); + } + + #[test] + fn subdivide_alpha_stays_linear_not_gamma_corrected() { + // Alpha is not gamma-encoded — it must keep lerping plainly, unlike + // RGB. + let a = Color4f::new(0.0, 0.0, 0.0, 0.0); + let b = Color4f::new(0.0, 0.0, 0.0, 1.0); + let (colors, _positions) = subdivide_gradient_stops(&[a, b], 1); + assert!( + (colors[1].a - 0.5).abs() < 1e-4, + "alpha midpoint should be a plain 0.5 lerp, got {}", + colors[1].a + ); + } +}