From d990678b32609e7f7301be4f389481c83c7eba96 Mon Sep 17 00:00:00 2001 From: Simon Koch-Sultan <126534658+scks1@users.noreply.github.com> Date: Sat, 31 Oct 2026 17:18:44 -0400 Subject: [PATCH 1/3] render: keep even-odd holes open in shadow, glow and reflection silhouettes effects_below merged all subpaths into one silhouette and filled it with the NonZero rule, discarding each subpath's even_odd flag, so the holes of donut, frame and similar preset shapes filled in under outer shadows, glows and reflections. Fill each participating subpath under its own fill rule at the three effect fill sites, mirroring fill_geometry; the glow stroke keeps the merged silhouette. Fixes #1596. The picture-content shadow clip path is unchanged (that is #1597's territory). --- crates/render/src/renderer.rs | 42 ++++++++++++++++++++++++----- crates/render/src/tests.rs | 50 +++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 7 deletions(-) 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(); From 9f952550b2b3b3a56b6b23e7b2b02eafcbe06e45 Mon Sep 17 00:00:00 2001 From: scks1 <126534658+scks1@users.noreply.github.com> Date: Sun, 1 Nov 2026 08:58:11 -0500 Subject: [PATCH 2/3] #1596: add shadow, glow and reflection pixel goldens --- crates/render/src/lib.rs | 3 + crates/render/src/tests_effect_holes.rs | 198 ++++++++++++++++++++++++ 2 files changed, 201 insertions(+) create mode 100644 crates/render/src/tests_effect_holes.rs 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/tests_effect_holes.rs b/crates/render/src/tests_effect_holes.rs new file mode 100644 index 00000000..10749229 --- /dev/null +++ b/crates/render/src/tests_effect_holes.rs @@ -0,0 +1,198 @@ +//! 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") { + use std::io::Write as _; + std::fs::create_dir_all(&dir).unwrap(); + let path = std::path::Path::new(&dir).join(format!("{shape}-{effect:?}.png")); + let mut file = std::fs::OpenOptions::new().write(true).create_new(true).open(path).unwrap(); + file.write_all(&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], + ], + ); +} From cc00a17dfe04194d9952d41b5e052efc634c1cda Mon Sep 17 00:00:00 2001 From: Simon Koch-Sultan <126534658+scks1@users.noreply.github.com> Date: Sun, 1 Nov 2026 15:52:54 -0500 Subject: [PATCH 3/3] #1596: let effect-hole QA images overwrite an earlier run's Co-Authored-By: Claude Opus 5.5 --- crates/render/src/tests_effect_holes.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/crates/render/src/tests_effect_holes.rs b/crates/render/src/tests_effect_holes.rs index 10749229..fdddf60f 100644 --- a/crates/render/src/tests_effect_holes.rs +++ b/crates/render/src/tests_effect_holes.rs @@ -64,11 +64,9 @@ 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") { - use std::io::Write as _; + // Overwrite, so a QA directory can be reused across runs. std::fs::create_dir_all(&dir).unwrap(); - let path = std::path::Path::new(&dir).join(format!("{shape}-{effect:?}.png")); - let mut file = std::fs::OpenOptions::new().write(true).create_new(true).open(path).unwrap(); - file.write_all(&image.to_png()).unwrap(); + std::fs::write(std::path::Path::new(&dir).join(format!("{shape}-{effect:?}.png")), image.to_png()).unwrap(); } image }