Merge pull request #2081 from TrevorBrown-dev/fix/ellipse-four-anchors-2080

Ellipse: an unsliced ellipse edits as four anchor points, not six (#2080)

crates/geom/src/lib.rs

@@ -474,15 +474,29 @@ pub fn ellipse(width: f64, height: f64, start: f64, sweep: f64, inner: f64) -> B
     let mut path = BezPath::new();
     // A whole turn as an arc with these radii: kurbo's `Ellipse` recovers its radii from an
     // affine map, which overflows to infinity for radii near f64's limit (and infinite cubics).
-    let whole = |rx: f64, ry: f64| kurbo::Arc::new(c, (rx, ry), 0.0, TAU, 0.0);
+    // Four cubics through the right, bottom, left and top extremes (clockwise from 3 o'clock),
+    // the same four anchors an ellipse has when edited as a vector; an arc subdivided by
+    // tolerance would give five or six.
+    let whole = |rx: f64, ry: f64| {
+        const K: f64 = 0.552_284_749_830_793_4;
+        let (kx, ky) = (rx * K, ry * K);
+        let mut p = BezPath::new();
+        p.move_to(c + Vec2::new(rx, 0.0));
+        p.curve_to(c + Vec2::new(rx, ky), c + Vec2::new(kx, ry), c + Vec2::new(0.0, ry));
+        p.curve_to(c + Vec2::new(-kx, ry), c + Vec2::new(-rx, ky), c + Vec2::new(-rx, 0.0));
+        p.curve_to(c + Vec2::new(-rx, -ky), c + Vec2::new(-kx, -ry), c + Vec2::new(0.0, -ry));
+        p.curve_to(c + Vec2::new(kx, -ry), c + Vec2::new(rx, -ky), c + Vec2::new(rx, 0.0));
+        p.close_path();
+        p
+    };
     if full && inner <= 0.0 {
-        return whole(rx, ry).to_path(tol);
+        return whole(rx, ry);
     }
     if full {
         // A ring: outer clockwise, inner counter-clockwise.
-        path.extend(whole(rx, ry).path_elements(tol));
-        let inner_path = whole(rx * inner, ry * inner).to_path(tol);
-        path.extend(inner_path.reverse_subpaths());
+        path.extend(whole(rx, ry).elements().iter().copied());
+        let inner_path = whole(rx * inner, ry * inner);
+        path.extend(inner_path.reverse_subpaths().elements().iter().copied());
         return path;
     }
     let outer = kurbo::Arc::new(c, (rx, ry), start, sweep, 0.0);

crates/geom/src/tests.rs

@@ -263,7 +263,7 @@ fn smoothed_polygon_smooths_every_corner() {
 #[test]
 fn ellipse_ring_area() {
     let full = area(&ellipse(100.0, 100.0, 0.0, TAU, 0.0));
-    assert!((full - PI * 2500.0).abs() < 2.0);
+    assert!((full - PI * 2500.0).abs() < 5.0, "{full}");
     let ring = ellipse(100.0, 100.0, 0.0, TAU, 0.5);
     // Winding area of a ring with a reversed inner contour: outer - inner.
     let a = ring.area().abs();
@@ -321,6 +321,46 @@ fn reverse_subpaths_preserves_open_and_closed() {
     assert!(matches!(r.elements().last(), Some(kurbo::PathEl::ClosePath)), "{:?}", r.elements());
 }
 
+fn cubics(path: &BezPath) -> Vec<kurbo::CubicBez> {
+    path.segments()
+        .filter_map(|s| match s {
+            kurbo::PathSeg::Cubic(c) => Some(c),
+            _ => None,
+        })
+        .collect()
+}
+
+#[test]
+fn a_whole_ellipse_is_four_cubics_through_the_extremes() {
+    for (w, h) in [(100.0, 100.0), (200.0, 60.0), (3.0, 900.0)] {
+        let segs = cubics(&ellipse(w, h, 0.0, TAU, 0.0));
+        assert_eq!(segs.len(), 4, "{w}x{h}");
+        let (rx, ry) = (w / 2.0, h / 2.0);
+        let want = [Point::new(rx, h), Point::new(0.0, ry), Point::new(rx, 0.0), Point::new(w, ry)];
+        for (i, s) in segs.iter().enumerate() {
+            assert!((s.p3 - want[i]).hypot() < 1e-9, "{w}x{h}: anchor {i} is {:?}", s.p3);
+            // Handle length is 0.5523 of the radius along the tangent.
+            let k = 0.552_284_749_830_793_4;
+            let r_along = if i % 2 == 0 { ry } else { rx };
+            assert!(((s.p1 - s.p0).hypot() - k * r_along).abs() < 1e-9, "{w}x{h}: handle {i}");
+        }
+    }
+}
+
+#[test]
+fn a_whole_ring_is_four_cubics_per_contour() {
+    let segs = cubics(&ellipse(100.0, 100.0, 0.0, TAU, 0.5));
+    assert_eq!(segs.len(), 8);
+}
+
+#[test]
+fn a_sliced_ellipse_still_builds_its_arc() {
+    let pie = ellipse(100.0, 100.0, 0.0, PI, 0.0);
+    assert!(!cubics(&pie).is_empty());
+    assert!(matches!(pie.elements().last(), Some(kurbo::PathEl::ClosePath)));
+    assert!((area(&pie) - PI * 1250.0).abs() < 2.0);
+}
+
 #[test]
 fn a_huge_ellipse_arc_is_a_few_cubics() {
     let big = ellipse(1e300, 1e300, 0.3, 2.0, 0.5);

crates/render/src/band_tests.rs

@@ -56,20 +56,61 @@ fn view(scale: f64) -> View {
     View { origin: (-10.0, -10.0), scale, width: (440.0 * scale) as u32, height: (240.0 * scale) as u32 }
 }
 
-/// Per row: the mean channel difference and how many pixels differ by more than 2. A band artifact
-/// differs along a whole row; edge antialiasing may differ in a few pixels per edge (a curve
-/// clipped at a band's edge rasterises a hair differently, most where it runs nearly level).
+/// A pixel whose largest channel differs by at most this much between two renders is edge
+/// antialiasing noise (a curve clipped at a band's edge rasterises a hair differently, most where
+/// it runs level, as at the top of an ellipse), not a band artifact.
+const NOISE: u8 = 24;
+
+/// One row of two renders: the channel differences of the pixels above noise (see `NOISE`) summed
+/// and divided by the whole row's channel count, so noise pixels add nothing but still count in
+/// the denominator; and how many pixels differ by more than 2 at all. A band artifact differs along
+/// a whole row, by more than noise.
+fn row_stats(x: &[u8], y: &[u8]) -> (f64, usize) {
+    let mut sum = 0.0;
+    let mut off = 0;
+    for (p, q) in x.chunks(4).zip(y.chunks(4)) {
+        let worst = p.iter().zip(q).map(|(c, d)| c.abs_diff(*d)).max().unwrap_or(0);
+        if worst > 2 {
+            off += 1;
+        }
+        if worst > NOISE {
+            sum += p.iter().zip(q).map(|(c, d)| f64::from(c.abs_diff(*d))).sum::<f64>();
+        }
+    }
+    (sum / x.len().max(1) as f64, off)
+}
+
 fn row_diffs(a: &Pixmap, b: &Pixmap) -> Vec<(f64, usize)> {
     let row = a.width() as usize * 4;
-    a.data()
-        .chunks(row)
-        .zip(b.data().chunks(row))
-        .map(|(x, y)| {
-            let sum: f64 = x.iter().zip(y).map(|(p, q)| f64::from(p.abs_diff(*q))).sum();
-            let off = x.chunks(4).zip(y.chunks(4)).filter(|(p, q)| p.iter().zip(*q).any(|(c, d)| c.abs_diff(*d) > 2)).count();
-            (sum / row as f64, off)
-        })
-        .collect()
+    a.data().chunks(row).zip(b.data().chunks(row)).map(|(x, y)| row_stats(x, y)).collect()
+}
+
+/// Whether a row passes: the original 0.25 limit on the mean from `row_stats`, and at most a
+/// twentieth of the row differing at all.
+fn row_ok(mean: f64, off: usize, width: usize) -> bool {
+    mean < 0.25 && off * 20 <= width
+}
+
+#[test]
+fn a_real_band_artifact_fails_but_edge_noise_passes() {
+    let width = 880; // scale 2
+    let flat = vec![100u8; width * 4];
+    // 44 pixels whose colour channels differ by 60 (alpha unchanged): the mean is
+    // 44 * 3 * 60 / (880 * 4) = 2.25, and the count, 44, is within a twentieth of the row. A real
+    // artifact, so it fails on the mean.
+    let mut artifact = flat.clone();
+    for px in artifact.chunks_mut(4).take(44) {
+        px.iter_mut().take(3).for_each(|c| *c += 60);
+    }
+    let (mean, off) = row_stats(&flat, &artifact);
+    assert!(!row_ok(mean, off, width), "{mean} {off}");
+    // 8 pixels differing by 16 (the ellipse top's antialiasing): forgiven.
+    let mut noise = flat.clone();
+    for px in noise.chunks_mut(4).take(8) {
+        px.iter_mut().for_each(|c| *c += 16);
+    }
+    let (mean, off) = row_stats(&flat, &noise);
+    assert!(row_ok(mean, off, width), "{mean} {off}");
 }
 
 #[test]
@@ -82,10 +123,7 @@ fn bands_match_one_piece_rendering() {
             let banded = render_bands(&d, p, &v, &Options::default(), bands).unwrap();
             assert_eq!((banded.width(), banded.height()), (one.width(), one.height()));
             for (y, (mean, off)) in row_diffs(&one, &banded).into_iter().enumerate() {
-                assert!(
-                    mean < 0.25 && off * 20 <= v.width as usize,
-                    "scale {scale}, {bands} bands: row {y} differs ({off} pixels, {mean:.2} on average)"
-                );
+                assert!(row_ok(mean, off, v.width as usize), "scale {scale}, {bands} bands: row {y} differs ({off} pixels, {mean:.2} on average)");
             }
         }
     }