diff --git a/crates/render/src/lib.rs b/crates/render/src/lib.rs index 5fd01141..15b89ec5 100644 --- a/crates/render/src/lib.rs +++ b/crates/render/src/lib.rs @@ -94,6 +94,9 @@ mod tests; #[cfg(test)] mod tests_wild; +#[cfg(test)] +mod tests_effect_holes; + #[cfg(test)] mod mt_tests { use super::*; diff --git a/crates/render/src/renderer.rs b/crates/render/src/renderer.rs index 84c5e5a8..c25f6bab 100644 --- a/crates/render/src/renderer.rs +++ b/crates/render/src/renderer.rs @@ -1125,12 +1125,15 @@ impl Renderer { // see-through as its fill (none for an outline-only shape) … None => { if let Some(a) = fill_alpha { - let mut filled = BezPath::new(); - for sp in geo.paths.iter().filter(|sp| sp.fill != FillMode::None) { - filled.extend(sp.path.iter()); - } ctx.set_paint(color(c.with_alpha(255), a)); - ctx.fill_path(&filled); + // Each subpath under its own fill rule, as the shape itself is + // drawn: one merged path filled NonZero fills the hole of an + // even-odd shape (a donut, a frame) (#1596). + for sp in geo.paths.iter().filter(|sp| sp.fill != FillMode::None) { + ctx.set_fill_rule(if sp.even_odd { peniko::Fill::EvenOdd } else { peniko::Fill::NonZero }); + ctx.fill_path(&sp.path); + } + ctx.set_fill_rule(peniko::Fill::NonZero); } } } @@ -1161,7 +1164,19 @@ impl Renderer { ctx.stroke_path(&silhouette); // A shape without a fill (an outline, a frame) glows along its line only (#225). if fill_alpha.is_some() || content.is_some() { - ctx.fill_path(&silhouette); + // Per subpath under its own fill rule, as in the shadow above: the + // merged silhouette filled NonZero fills an even-odd hole (#1596). + // When no subpath takes part the silhouette is the outline fallback. + let mut filled_any = false; + for sp in geo.paths.iter().filter(|sp| sp.fill != FillMode::None || fill.is_none()) { + ctx.set_fill_rule(if sp.even_odd { peniko::Fill::EvenOdd } else { peniko::Fill::NonZero }); + ctx.fill_path(&sp.path); + filled_any = true; + } + if !filled_any { + ctx.fill_path(&silhouette); + } + ctx.set_fill_rule(peniko::Fill::NonZero); } ctx.pop_layer(); } @@ -1180,7 +1195,20 @@ impl Renderer { } None => { if let Some(fl) = fill { - paint::fill_path(ctx, rctx, fl, ph, &silhouette, Rect::new(0.0, 0.0, w, h), mm, FillMode::Norm, 1.0, rctx.pres); + // Per subpath under its own fill rule, as in the shadow above: + // the merged silhouette filled NonZero fills an even-odd hole + // (#1596). The outline fallback covers shapes with no + // participating subpath. + let mut filled_any = false; + for sp in geo.paths.iter().filter(|sp| sp.fill != FillMode::None || fill.is_none()) { + ctx.set_fill_rule(if sp.even_odd { peniko::Fill::EvenOdd } else { peniko::Fill::NonZero }); + paint::fill_path(ctx, rctx, fl, ph, &sp.path, Rect::new(0.0, 0.0, w, h), mm, FillMode::Norm, 1.0, rctx.pres); + filled_any = true; + } + if !filled_any { + paint::fill_path(ctx, rctx, fl, ph, &silhouette, Rect::new(0.0, 0.0, w, h), mm, FillMode::Norm, 1.0, rctx.pres); + } + ctx.set_fill_rule(peniko::Fill::NonZero); } } } diff --git a/crates/render/src/tests.rs b/crates/render/src/tests.rs index f01ee1cf..2de2daab 100644 --- a/crates/render/src/tests.rs +++ b/crates/render/src/tests.rs @@ -515,6 +515,56 @@ fn glow_of_a_shape_with_no_fill_and_no_line_is_not_drawn() { } } +#[test] +fn shadow_and_glow_keep_an_even_odd_shapes_hole() { + // #1596: a donut is one even-odd subpath — its hole exists only under the + // even-odd rule. The effects were cast from a merged silhouette filled + // NonZero, so the shadow and the glow filled the hole solid. The hole's + // centre must stay the slide's white in both, while the effects still draw. + let donut = |effects: deckcraft_model::Effects| { + let mut sh = Shape { + id: ShapeId(612), + xfrm: Some(Xfrm::new(50.0, 50.0, 200.0, 200.0)), + geom: Geom::preset("donut"), + fill: Some(Fill::solid(ColorRef::rgb(deckcraft_color::Rgba::rgb(255, 160, 0)))), + ..Default::default() + }; + sh.effects = Some(effects); + render_slide(&deck_with(vec![sh]), 0, &RenderOpts { scale: 1.0, ..Default::default() }) + }; + let img = donut(deckcraft_model::Effects { + outer_shadow: Some(deckcraft_model::style::Shadow { + color: ColorRef::rgb(deckcraft_color::Rgba::BLACK), + blur: 8.0, + dist: 0.0, + dir: 45.0, + inner: false, + sx: 1.0, + sy: 1.0, + kx: 0.0, + ky: 0.0, + align: String::new(), + rotate_with_shape: false, + }), + ..Default::default() + }); + let hole = img.pixel(150, 150); + assert!(hole[0] > 245 && hole[1] > 245 && hole[2] > 245, "shadow in the hole: {hole:?}"); + // The shadow still falls around the shape (just below its bottom edge). + let below = img.pixel(150, 251); + assert!(below[0] < 200 && below[0] == below[2], "no shadow below: {below:?}"); + + let img = donut(deckcraft_model::Effects { + glow: Some(deckcraft_model::style::Glow { color: ColorRef::rgb(deckcraft_color::Rgba::rgb(255, 0, 0)), radius: 8.0 }), + ..Default::default() + }); + let hole = img.pixel(150, 150); + assert!(hole[0] > 245 && hole[1] > 245 && hole[2] > 245, "glow in the hole: {hole:?}"); + // The ring itself is the shape's orange. + let ring = img.pixel(150, 75); + assert!(ring[0] > 200 && ring[1] > 100 && ring[2] < 100, "ring: {ring:?}"); +} + #[test] fn placeholder_prompts_only_in_edit_view() { let p = Presentation::default(); diff --git a/crates/render/src/tests_effect_holes.rs b/crates/render/src/tests_effect_holes.rs new file mode 100644 index 00000000..fdddf60f --- /dev/null +++ b/crates/render/src/tests_effect_holes.rs @@ -0,0 +1,196 @@ +//! Contributor-original pixel goldens for #1596 and the review of #1626. +//! The fixed samples cover background, hole/centre, ring, visible halos and reflected coverage. + +use super::{Fill, Image, RenderOpts, Rgba, render_slide}; +use deckcraft_model::style::{ColorRef, Effects, Glow, Line, Reflection, Shadow}; +use deckcraft_model::{Geom, Presentation, Shape, ShapeId, Xfrm}; + +const PROBES: [(u32, u32); 9] = [ + (20, 20), // Clear background. + (130, 130), // Clear donut hole; filled ellipse centre. + (130, 70), // Original ring/ellipse fill. + (130, 47), // Visible shadow/glow above the outer edge. + (130, 213), // Visible shadow/glow below the outer edge. + (130, 93), // Shadow/glow along the inner rim, inside the hole. + (130, 230), // Reflected upper ring. + (130, 300), // Reflected hole/ellipse centre. + (70, 300), // Reflected ring at the same fade as the hole. +]; + +#[derive(Clone, Copy, Debug)] +enum Effect { + Shadow, + Glow, + Reflection, +} + +fn scene(shape: &str, effect: Effect) -> Presentation { + let mut p = Presentation::default(); + let mut effects = Effects::default(); + match effect { + Effect::Shadow => { + effects.outer_shadow = Some(Shadow { + color: ColorRef::rgb(Rgba::BLACK), + blur: 8.0, + dist: 0.0, + dir: 45.0, + inner: false, + sx: 1.0, + sy: 1.0, + kx: 0.0, + ky: 0.0, + align: String::new(), + rotate_with_shape: false, + }) + } + Effect::Glow => effects.glow = Some(Glow { color: ColorRef::rgb(Rgba::rgb(255, 0, 0)), radius: 8.0 }), + Effect::Reflection => effects.reflection = Some(Reflection { blur: 0.0, start_alpha: 1.0, end_alpha: 0.0, end_pos: 1.0, dist: 10.0 }), + } + let slide = std::sync::Arc::make_mut(&mut p.slides[0]); + slide.background = Some(deckcraft_model::Background::Fill { fill: Fill::solid(ColorRef::rgb(Rgba::WHITE)) }); + slide.shapes = vec![Shape { + id: ShapeId(1596), + xfrm: Some(Xfrm::new(50.0, 50.0, 160.0, 160.0)), + geom: Geom::preset(shape), + fill: Some(Fill::solid(ColorRef::rgb(Rgba::rgb(32, 96, 224)))), + line: Some(Line::none()), + effects: Some(effects), + ..Default::default() + }]; + p +} + +fn draw(shape: &str, effect: Effect) -> Image { + let p = scene(shape, effect); + let image = render_slide(&p, 0, &RenderOpts { scale: 1.0, ..Default::default() }); + if let Ok(dir) = std::env::var("DECKCRAFT_EFFECT_HOLES_QA") { + // Overwrite, so a QA directory can be reused across runs. + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(std::path::Path::new(&dir).join(format!("{shape}-{effect:?}.png")), image.to_png()).unwrap(); + } + image +} + +// Fixed contributor-original RGBA goldens; tolerate one final quantization step. +// The nonwhite halo/reflection samples also fail if an effect is disabled outright. +fn assert_golden(shape: &str, effect: Effect, expected: [[u8; 4]; 9]) { + let image = draw(shape, effect); + for ((x, y), want) in PROBES.into_iter().zip(expected) { + let got = image.pixel(x, y); + assert!(got.iter().zip(want).all(|(a, b)| (*a as i32 - b as i32).abs() <= 1), "{shape} {effect:?} at {x},{y}: {got:?} vs golden {want:?}"); + } +} + +#[test] +fn donut_shadow_matches_golden() { + assert_golden( + "donut", + Effect::Shadow, + [ + [255, 255, 255, 255], + [255, 255, 255, 255], + [32, 96, 224, 255], + [214, 214, 214, 255], + [233, 233, 233, 255], + [227, 227, 227, 255], + [255, 255, 255, 255], + [255, 255, 255, 255], + [255, 255, 255, 255], + ], + ); +} + +#[test] +fn donut_glow_matches_golden() { + assert_golden( + "donut", + Effect::Glow, + [ + [255, 255, 255, 255], + [255, 255, 255, 255], + [32, 96, 224, 255], + [255, 49, 49, 255], + [255, 111, 111, 255], + [255, 96, 96, 255], + [255, 255, 255, 255], + [255, 255, 255, 255], + [255, 255, 255, 255], + ], + ); +} + +#[test] +fn donut_reflection_matches_golden() { + assert_golden( + "donut", + Effect::Reflection, + [ + [255, 255, 255, 255], + [255, 255, 255, 255], + [32, 96, 224, 255], + [255, 255, 255, 255], + [255, 255, 255, 255], + [255, 255, 255, 255], + [130, 166, 238, 255], + [255, 255, 255, 255], + [189, 208, 246, 255], + ], + ); +} + +#[test] +fn ellipse_shadow_matches_golden() { + assert_golden( + "ellipse", + Effect::Shadow, + [ + [255, 255, 255, 255], + [32, 96, 224, 255], + [32, 96, 224, 255], + [214, 214, 214, 255], + [233, 233, 233, 255], + [32, 96, 224, 255], + [255, 255, 255, 255], + [255, 255, 255, 255], + [255, 255, 255, 255], + ], + ); +} + +#[test] +fn ellipse_glow_matches_golden() { + assert_golden( + "ellipse", + Effect::Glow, + [ + [255, 255, 255, 255], + [32, 96, 224, 255], + [32, 96, 224, 255], + [255, 49, 49, 255], + [255, 111, 111, 255], + [32, 96, 224, 255], + [255, 255, 255, 255], + [255, 255, 255, 255], + [255, 255, 255, 255], + ], + ); +} + +#[test] +fn ellipse_reflection_matches_golden() { + assert_golden( + "ellipse", + Effect::Reflection, + [ + [255, 255, 255, 255], + [32, 96, 224, 255], + [32, 96, 224, 255], + [255, 255, 255, 255], + [255, 255, 255, 255], + [32, 96, 224, 255], + [130, 166, 238, 255], + [189, 208, 246, 255], + [189, 208, 246, 255], + ], + ); +}