From eccce74c4b260379874142e88146082bb01cc754 Mon Sep 17 00:00:00 2001
From: Chad Brokaw <cbrokaw@gmail.com>
Date: Wed, 23 Nov 2022 16:51:30 -0500
Subject: [PATCH] address review feedback

---
 piet-gpu/src/samples.rs         | 18 ++++++--------
 piet-scene/src/scene/builder.rs | 42 ++++++++++++++++++++++-----------
 2 files changed, 35 insertions(+), 25 deletions(-)

diff --git a/piet-gpu/src/samples.rs b/piet-gpu/src/samples.rs
index f0629ae..fd6ea6b 100644
--- a/piet-gpu/src/samples.rs
+++ b/piet-gpu/src/samples.rs
@@ -1,5 +1,5 @@
 use crate::PicoSvg;
-use piet_scene::kurbo::{Affine, Ellipse, PathEl, Point, Rect};
+use piet_scene::kurbo::{Affine, BezPath, Ellipse, PathEl, Point, Rect};
 use piet_scene::*;
 
 use crate::SimpleText;
@@ -104,7 +104,7 @@ fn render_cardioid(sb: &mut SceneBuilder) {
     let dth = std::f64::consts::PI * 2.0 / (n as f64);
     let center = Point::new(1024.0, 768.0);
     let r = 750.0;
-    let mut path = vec![];
+    let mut path = BezPath::new();
     for i in 1..n {
         let mut p0 = center;
         let a0 = i as f64 * dth;
@@ -122,7 +122,7 @@ fn render_cardioid(sb: &mut SceneBuilder) {
         Affine::IDENTITY,
         Color::rgb8(0, 0, 0),
         None,
-        &&path[..],
+        &path,
     );
 }
 
@@ -169,26 +169,22 @@ fn render_alpha_test(sb: &mut SceneBuilder) {
         Affine::IDENTITY,
         Color::rgb8(255, 0, 0),
         None,
-        &&make_diamond(1024.0, 100.0)[..],
+        &make_diamond(1024.0, 100.0),
     );
     sb.fill(
         Fill::NonZero,
         Affine::IDENTITY,
         Color::rgba8(0, 255, 0, 0x80),
         None,
-        &&make_diamond(1024.0, 125.0)[..],
-    );
-    sb.push_layer(
-        Mix::Clip,
-        Affine::IDENTITY,
-        &&make_diamond(1024.0, 150.0)[..],
+        &make_diamond(1024.0, 125.0),
     );
+    sb.push_layer(Mix::Clip, Affine::IDENTITY, &make_diamond(1024.0, 150.0));
     sb.fill(
         Fill::NonZero,
         Affine::IDENTITY,
         Color::rgba8(0, 0, 255, 0x80),
         None,
-        &&make_diamond(1024.0, 175.0)[..],
+        &make_diamond(1024.0, 175.0),
     );
     sb.pop_layer();
 }
diff --git a/piet-scene/src/scene/builder.rs b/piet-scene/src/scene/builder.rs
index 606fb49..17a236a 100644
--- a/piet-scene/src/scene/builder.rs
+++ b/piet-scene/src/scene/builder.rs
@@ -17,14 +17,14 @@
 use super::{conv, Scene, SceneData, SceneFragment};
 use crate::ResourcePatch;
 use bytemuck::{Pod, Zeroable};
-use peniko::kurbo::{Affine, PathEl, Shape};
+use peniko::kurbo::{Affine, PathEl, Rect, Shape};
 use peniko::{BlendMode, BrushRef, ColorStop, Fill, Stroke};
 use smallvec::SmallVec;
 
 /// Builder for constructing a scene or scene fragment.
 pub struct SceneBuilder<'a> {
     scene: &'a mut SceneData,
-    layers: SmallVec<[(BlendMode, bool); 8]>,
+    layers: SmallVec<[BlendMode; 8]>,
 }
 
 impl<'a> SceneBuilder<'a> {
@@ -60,23 +60,19 @@ impl<'a> SceneBuilder<'a> {
         let blend = blend.into();
         self.maybe_encode_transform(transform);
         self.linewidth(-1.0);
-        if self.encode_path(shape, true) {
-            self.begin_clip(blend);
-            self.layers.push((blend, true));
-        } else {
-            // When the layer has an empty path, record an entry to prevent
-            // the stack from becoming unbalanced. This is handled in
-            // pop_layer.
-            self.layers.push((blend, false));
+        if !self.encode_path(shape, true) {
+            // If the layer shape is invalid, encode a valid empty path. This suppresses
+            // all drawing until the layer is popped.
+            self.encode_path(&Rect::new(0.0, 0.0, 0.0, 0.0), true);
         }
+        self.begin_clip(blend);
+        self.layers.push(blend);
     }
 
     /// Pops the current layer.
     pub fn pop_layer(&mut self) {
-        if let Some((blend, active)) = self.layers.pop() {
-            if active {
-                self.end_clip(blend);
-            }
+        if let Some(blend) = self.layers.pop() {
+            self.end_clip(blend);
         }
     }
 
@@ -138,6 +134,10 @@ impl<'a> SceneBuilder<'a> {
 }
 
 impl<'a> SceneBuilder<'a> {
+    /// Encodes a path for the specified shape.
+    ///
+    /// When the `is_fill` parameter is true, closes any open subpaths by inserting
+    /// a line to the start point of the subpath with the end segment bit set.
     fn encode_path(&mut self, shape: &impl Shape, is_fill: bool) -> bool {
         let mut b = PathBuilder::new(
             &mut self.scene.tag_stream,
@@ -378,6 +378,12 @@ impl<'a> PathBuilder<'a> {
 
     pub fn line_to(&mut self, x: f32, y: f32) {
         if self.state == PathState::Start {
+            if self.n_pathseg == 0 {
+                // This copies the behavior of kurbo which treats an initial line, quad
+                // or curve as a move.
+                self.move_to(x, y);
+                return;
+            }
             self.move_to(self.first_pt[0], self.first_pt[1]);
         }
         let buf = [x, y];
@@ -390,6 +396,10 @@ impl<'a> PathBuilder<'a> {
 
     pub fn quad_to(&mut self, x1: f32, y1: f32, x2: f32, y2: f32) {
         if self.state == PathState::Start {
+            if self.n_pathseg == 0 {
+                self.move_to(x2, y2);
+                return;
+            }
             self.move_to(self.first_pt[0], self.first_pt[1]);
         }
         let buf = [x1, y1, x2, y2];
@@ -402,6 +412,10 @@ impl<'a> PathBuilder<'a> {
 
     pub fn cubic_to(&mut self, x1: f32, y1: f32, x2: f32, y2: f32, x3: f32, y3: f32) {
         if self.state == PathState::Start {
+            if self.n_pathseg == 0 {
+                self.move_to(x3, y3);
+                return;
+            }
             self.move_to(self.first_pt[0], self.first_pt[1]);
         }
         let buf = [x1, y1, x2, y2, x3, y3];