Various local changes for Vello API

sparse_strips/vello_api/README.md

@@ -113,7 +113,7 @@ This includes:
 - Renderer specific painting commands (i.e. using downcasting).
   This is intended to be an immediate follow-up to the MVP landing.
 - Pushing/popping clip paths (i.e. non-isolated clipping).
-  This feature should be easy to restore, although it isn't clear how it will work with "Vello GPU", i.e. Hybrid with GPU rasterisation
+  This feature should be easy to restore, although it isn't clear how it will work with "Vello GPU", i.e. Hybrid with GPU sparse strip rendering.
 - Downloading rendered textures back to the CPU/host.
   This is currently supported through individual methods on the renderers, but we hope to have a portable API for coordinating this.
 - Anti-aliasing threshold.

sparse_strips/vello_api/src/lib.rs

@@ -93,7 +93,7 @@
 //! - Renderer specific painting commands (i.e. using downcasting).
 //!   This is intended to be an immediate follow-up to the MVP landing.
 //! - Pushing/popping clip paths (i.e. non-isolated clipping).
-//!   This feature should be easy to restore, although it isn't clear how it will work with "Vello GPU", i.e. Hybrid with GPU rasterisation
+//!   This feature should be easy to restore, although it isn't clear how it will work with "Vello GPU", i.e. Hybrid with GPU sparse strip rendering.
 //! - Downloading rendered textures back to the CPU/host.
 //!   This is currently supported through individual methods on the renderers, but we hope to have a portable API for coordinating this.
 //! - Anti-aliasing threshold.

sparse_strips/vello_api/src/painter.rs

@@ -73,18 +73,18 @@ pub trait PaintScene: Any {
     ///   For hinted scenes, the only valid transforms are integer translations.
     /// - `scene` does not apply to the same renderer as `self`.
     /// - `scene` has unbalanced layers (TODO: This isn't implemented yet).
+    // TODO: The reason this method doesn't have a default impl is because of future support for custom paint commands.
+    // However, it might be possible to also implement those directly in a generic impl.
     fn append(&mut self, transform: Affine, scene: &Scene) -> Result<(), ()>;
 
-    /// Fill the interior of the closed path described by `path` with the current brush.
+    /// Fill the interior of `shape` with the current brush.
     ///
-    /// Both `path` and the current brush will be transformed using the 2d affine `transform`.
-    /// For self-intersecting or nested paths, the areas which are treated as the interior
-    /// will be determined using the given `fill_rule`.
-    /// See the documentation on [`Fill`]'s variants for details on when you might choose a given
-    /// fill rule.
+    /// The shape may contain multiple subpaths (e.g., for shapes with holes like a donut).
+    /// Each subpath is implicitly closed if needed with a straight line.
+    /// For self-intersecting or nested subpaths, the `fill_rule` determines which areas are considered interior.
     ///
-    /// If `path` is not a closed path, a fallback straight-line closing segment may be appended.
-    /// (TODO: Should we give a warning of some kind as well?)
+    /// Both `shape` and the current brush will be transformed using the 2d affine `transform`.
+    /// See the documentation on [`Fill`]'s variants for details on when you might choose a given fill rule.
     // It would be really nice to have images of nested paths here, to explain winding numbers.
     fn fill_path(&mut self, transform: Affine, fill_rule: Fill, path: impl Shape);
 

sparse_strips/vello_api/src/scene.rs

@@ -149,11 +149,13 @@ impl Scene {
 pub fn extract_integer_translation(transform: Affine) -> Option<(f64, f64)> {
     fn is_nearly(a: f64, b: f64) -> bool {
         // TODO: This is a very arbitrary threshold.
+        // It's valid for it to be as high as it is, because this is in units of a pixel,
+        // so 1/100th of a pixel is negligible.
         (a - b).abs() < 0.01
     }
     let [a, b, c, d, dx, dy] = transform.as_coeffs();
     // If there's a skew, rotation or scale, then the transform is not compatible with hinting.
-    if !(is_nearly(a, 1.0) && is_nearly(b, 0.0) && is_nearly(c, 1.0) && is_nearly(d, 0.0)) {
+    if !(is_nearly(a, 1.0) && is_nearly(b, 0.0) && is_nearly(c, 0.0) && is_nearly(d, 1.0)) {
         return None;
     }
 
@@ -232,6 +234,7 @@ impl PaintScene for Scene {
         let idx = self.paths.prepare_shape(&path, fill_rule);
         self.commands.push(RenderCommand::DrawPath(transform, idx));
     }
+
     fn stroke_path(
         &mut self,
         transform: Affine,
@@ -292,7 +295,55 @@ impl PaintScene for Scene {
                 opacity,
             }));
     }
+
     fn pop_layer(&mut self) {
         self.commands.push(RenderCommand::PopLayer);
     }
 }
+
+#[cfg(test)]
+mod test {
+    use core::f64::consts::{FRAC_PI_2, FRAC_PI_3, PI};
+
+    use peniko::kurbo::Affine;
+
+    use crate::scene::extract_integer_translation;
+
+    #[test]
+    fn integer_translations() {
+        let coords = [
+            (10., 10.),
+            (1.00001, 1.),
+            (0.99999998, 1.),
+            (0.0000001, 0.),
+            (10_000., 10_000.),
+        ];
+        for (real_x, rounded_x) in coords {
+            for (real_y, rounded_y) in coords {
+                let xform = Affine::translate((real_x, real_y));
+                let (extracted_x, extracted_y) = extract_integer_translation(xform)
+                    .expect("Passed coordinates are all near integers.");
+                assert_eq!(extracted_x, rounded_x);
+                assert_eq!(extracted_y, rounded_y);
+            }
+        }
+    }
+
+    #[test]
+    fn unhintable_transforms() {
+        let transforms = [
+            Affine::translate((10.5, 0.)),
+            Affine::skew(1.0, 0.5),
+            // Technically, PI/2 and PI *could* be hinted, but they can't reuse the cached strips.
+            Affine::rotate(FRAC_PI_2),
+            Affine::rotate(PI),
+            Affine::rotate(FRAC_PI_3),
+        ];
+        for xform in transforms {
+            assert!(
+                extract_integer_translation(xform).is_none(),
+                "{xform:?} unexpectedly was treated as hintable."
+            );
+        }
+    }
+}

sparse_strips/vello_cpu/src/api.rs

@@ -91,7 +91,7 @@ impl PaintScene for CPUScenePainter {
                             .get(usize::try_from(path_id.0).unwrap() + 1)
                             .map_or(input_paths.elements.len(), |it| it.start_index);
                         let segments = &input_paths.elements[path.start_index..*path_end];
-                        // Obviously, ideally we'd not be allocating here. This is forced by the current public API of Vello CPU.
+                        // TODO: Obviously, ideally we'd not be allocating here. This is forced by the current public API of Vello CPU.
                         let bezpath = BezPath::from_iter(segments.iter().cloned());
                         Some(bezpath)
                     } else {
@@ -137,13 +137,13 @@ impl PaintScene for CPUScenePainter {
         self.render_context.set_transform(transform);
         self.render_context.set_fill_rule(fill_rule);
         // TODO: Tweak inner `fill_path` API to either take a `Shape` or an &[PathEl]
-        self.render_context.fill_path(&path.to_path(0.1));
+        self.render_context.fill_path(&path.into_path(0.1));
     }
 
     fn stroke_path(&mut self, transform: Affine, stroke_params: &kurbo::Stroke, path: impl Shape) {
         self.render_context.set_transform(transform);
         self.render_context.set_stroke(stroke_params.clone());
-        self.render_context.stroke_path(&path.to_path(0.1));
+        self.render_context.stroke_path(&path.into_path(0.1));
     }
 
     fn set_brush(
@@ -208,7 +208,7 @@ impl PaintScene for CPUScenePainter {
         self.render_context.set_fill_rule(Fill::NonZero);
         self.render_context.set_transform(clip_transform);
         self.render_context.push_layer(
-            clip_path.map(|it| it.to_path(0.1)).as_ref(),
+            clip_path.map(|it| it.into_path(0.1)).as_ref(),
             blend_mode,
             opacity,
             None,
@@ -221,7 +221,7 @@ impl PaintScene for CPUScenePainter {
         self.render_context.set_transform(clip_transform);
         self.render_context.push_clip_layer(
             // TODO: Not allocate
-            &path.to_path(0.1),
+            &path.into_path(0.1),
         );
     }
 

sparse_strips/vello_hybrid/src/api.rs

@@ -89,7 +89,7 @@ impl PaintScene for HybridScenePainter {
                             .get(usize::try_from(path_id.0).unwrap() + 1)
                             .map_or(input_paths.elements.len(), |it| it.start_index);
                         let segments = &input_paths.elements[path.start_index..*path_end];
-                        // Obviously, ideally we'd not be allocating here. This is forced by the current public API of Vello CPU.
+                        // TODO: Obviously, ideally we'd not be allocating here. This is forced by the current public API of Vello CPU.
                         let bezpath = BezPath::from_iter(segments.iter().cloned());
                         Some(bezpath)
                     } else {
@@ -135,13 +135,13 @@ impl PaintScene for HybridScenePainter {
         self.scene.set_transform(transform);
         self.scene.set_fill_rule(fill_rule);
         // TODO: Tweak inner `fill_path` API to either take a `Shape` or an &[PathEl]
-        self.scene.fill_path(&path.to_path(0.1));
+        self.scene.fill_path(&path.into_path(0.1));
     }
 
     fn stroke_path(&mut self, transform: Affine, stroke_params: &kurbo::Stroke, path: impl Shape) {
         self.scene.set_transform(transform);
         self.scene.set_stroke(stroke_params.clone());
-        self.scene.stroke_path(&path.to_path(0.1));
+        self.scene.stroke_path(&path.into_path(0.1));
     }
 
     fn set_brush(
@@ -184,9 +184,12 @@ impl PaintScene for HybridScenePainter {
         opacity: Option<f32>,
         // mask: Option<Mask>,
     ) {
+        // We set the fill rule to nonzero for the clip path as a reasonable default.
+        // We should make it user provided in the future
+        self.scene.set_fill_rule(Fill::NonZero);
         self.scene.set_transform(clip_transform);
         self.scene.push_layer(
-            clip_path.map(|it| it.to_path(0.1)).as_ref(),
+            clip_path.map(|it| it.into_path(0.1)).as_ref(),
             blend_mode,
             opacity,
             None,
@@ -198,7 +201,7 @@ impl PaintScene for HybridScenePainter {
         self.scene.set_transform(clip_transform);
         self.scene.push_clip_layer(
             // TODO: Not allocate
-            &path.to_path(0.1),
+            &path.into_path(0.1),
         );
     }