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] 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();