From a05c7302f0122aea68b9509ef1564a02b6ef3e7f Mon Sep 17 00:00:00 2001 From: Raph Levien Date: Thu, 26 Sep 2024 16:32:01 -0400 Subject: [PATCH 1/6] Match CPU/GPU logic for start tangents When encoding a start tangent for an end cap or closed segment, use logic designed to match the GPU computed tangent for the first segment. Note: this changes the GPU calculation to write out the lerp calculation explicitly rather than use the mix intrinsic, so we can rely on the rounding behavior. In the presence of fastmath, the rounding behavior is not guaranteed, but it is verified to work on M1. Fixes #704, unless we get bitten by fastmath. --- vello_encoding/src/path.rs | 52 ++++++++++++++++++++++++------- vello_shaders/shader/flatten.wgsl | 8 ++--- 2 files changed, 44 insertions(+), 16 deletions(-) diff --git a/vello_encoding/src/path.rs b/vello_encoding/src/path.rs index f5518d030..be1baae16 100644 --- a/vello_encoding/src/path.rs +++ b/vello_encoding/src/path.rs @@ -524,7 +524,7 @@ impl<'a> PathEncoder<'a> { } if self.state == PathState::MoveTo { // Ensure that we don't end up with a zero-length start tangent. - let Some((x, y)) = self.start_tangent_for_curve((x, y), None, None) else { + let Some((x, y)) = self.start_tangent_for_line((x, y)) else { return; }; self.first_start_tangent_end = [x, y]; @@ -552,7 +552,7 @@ impl<'a> PathEncoder<'a> { } if self.state == PathState::MoveTo { // Ensure that we don't end up with a zero-length start tangent. - let Some((x, y)) = self.start_tangent_for_curve((x1, y1), Some((x2, y2)), None) else { + let Some((x, y)) = self.start_tangent_for_quad((x1, y1), (x2, y2)) else { return; }; self.first_start_tangent_end = [x, y]; @@ -580,9 +580,7 @@ impl<'a> PathEncoder<'a> { } if self.state == PathState::MoveTo { // Ensure that we don't end up with a zero-length start tangent. - let Some((x, y)) = - self.start_tangent_for_curve((x1, y1), Some((x2, y2)), Some((x3, y3))) - else { + let Some((x, y)) = self.start_tangent_for_curve((x1, y1), (x2, y2), (x3, y3)) else { return; }; self.first_start_tangent_end = [x, y]; @@ -748,20 +746,17 @@ impl<'a> PathEncoder<'a> { } // Returns the end point of the start tangent of a curve starting at `(x0, y0)`, or `None` if the - // curve is degenerate / has zero-length. The inputs are a sequence of control points that can - // represent a line, a quadratic Bezier, or a cubic Bezier. Lines and quadratic Beziers can be - // passed to this function by simply setting the invalid control point degrees equal to `None`. + // curve is degenerate / has zero-length. The inputs are a sequence of control points that + // represent a cubic Bezier. // // `self.first_point` is always treated as the first control point of the curve. fn start_tangent_for_curve( &self, p1: (f32, f32), - p2: Option<(f32, f32)>, - p3: Option<(f32, f32)>, + p2: (f32, f32), + p3: (f32, f32), ) -> Option<(f32, f32)> { let p0 = (self.first_point[0], self.first_point[1]); - let p2 = p2.unwrap_or(p0); - let p3 = p3.unwrap_or(p0); let pt = if (p1.0 - p0.0).abs() > EPSILON || (p1.1 - p0.1).abs() > EPSILON { p1 } else if (p2.0 - p0.0).abs() > EPSILON || (p2.1 - p0.1).abs() > EPSILON { @@ -773,6 +768,39 @@ impl<'a> PathEncoder<'a> { }; Some(pt) } + + // Similar to `start_tangent_for_curve` but for a line. + fn start_tangent_for_line(&self, p1: (f32, f32)) -> Option<(f32, f32)> { + let p0 = (self.first_point[0], self.first_point[1]); + let pt = if (p1.0 - p0.0).abs() > EPSILON || (p1.1 - p0.1).abs() > EPSILON { + ( + p0.0 + 1. / 3. * (p1.0 - p0.0), + p0.1 + 1. / 3. * (p1.1 - p0.1), + ) + } else { + return None; + }; + Some(pt) + } + + // Similar to `start_tangent_for_curve` but for a quadratic Bézier. + fn start_tangent_for_quad(&self, p1: (f32, f32), p2: (f32, f32)) -> Option<(f32, f32)> { + let p0 = (self.first_point[0], self.first_point[1]); + let pt = if (p1.0 - p0.0).abs() > EPSILON || (p1.1 - p0.1).abs() > EPSILON { + ( + p1.0 + 1. / 3. * (p0.0 - p1.0), + p1.1 + 1. / 3. * (p0.1 - p1.1), + ) + } else if (p2.0 - p0.0).abs() > EPSILON || (p2.1 - p0.1).abs() > EPSILON { + ( + p1.0 + 1. / 3. * (p2.0 - p1.0), + p1.1 + 1. / 3. * (p2.1 - p1.1), + ) + } else { + return None; + }; + Some(pt) + } } #[cfg(feature = "full")] diff --git a/vello_shaders/shader/flatten.wgsl b/vello_shaders/shader/flatten.wgsl index cd42fdeb3..f6266ab72 100644 --- a/vello_shaders/shader/flatten.wgsl +++ b/vello_shaders/shader/flatten.wgsl @@ -735,12 +735,12 @@ fn read_path_segment(tag: PathTagData, is_stroke: bool) -> CubicPoints { // Degree-raise if seg_type == PATH_TAG_LINETO { p3 = p1; - p2 = mix(p3, p0, 1.0 / 3.0); - p1 = mix(p0, p3, 1.0 / 3.0); + p2 = p3 + (1.0 / 3.0) * (p0 - p3); + p1 = p0 + (1.0 / 3.0) * (p3 - p0); } else if seg_type == PATH_TAG_QUADTO { p3 = p2; - p2 = mix(p1, p2, 1.0 / 3.0); - p1 = mix(p1, p0, 1.0 / 3.0); + p2 = p1 + (1.0 / 3.0) * (p2 - p1); + p1 = p1 + (1.0 / 3.0) * (p0 - p1); } return CubicPoints(p0, p1, p2, p3); From af3e0cc41353d0a0452d0edfb7dbfcf752ff01bf Mon Sep 17 00:00:00 2001 From: Raph Levien Date: Wed, 2 Oct 2024 13:57:03 -0700 Subject: [PATCH 2/6] Use fma() for affine transforms Use the explicit `fma()` intrinsic to express affine transformations. This reduces the chance that the compiler will compile different instances of the same source expression with different association/rounding behavior, and in particular reduces the incidence of watertightness failures with a rotation transform. --- vello_shaders/shader/flatten.wgsl | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/vello_shaders/shader/flatten.wgsl b/vello_shaders/shader/flatten.wgsl index f6266ab72..d3a9012a4 100644 --- a/vello_shaders/shader/flatten.wgsl +++ b/vello_shaders/shader/flatten.wgsl @@ -649,7 +649,9 @@ fn read_transform(transform_base: u32, ix: u32) -> Transform { } fn transform_apply(transform: Transform, p: vec2f) -> vec2f { - return transform.mat.xy * p.x + transform.mat.zw * p.y + transform.translate; + let px = fma(transform.mat.x, p.x, fma(transform.mat.z, p.y, transform.translate.x)); + let py = fma(transform.mat.y, p.x, fma(transform.mat.w, p.y, transform.translate.y)); + return vec2(px, py); } fn round_down(x: f32) -> i32 { From a0af67e93d107e2bc66aa1f85603b976f32141c8 Mon Sep 17 00:00:00 2001 From: Raph Levien Date: Wed, 2 Oct 2024 14:53:42 -0700 Subject: [PATCH 3/6] Add longpathdash test This adds a test. It's clipped to 250x250 otherwise it would hit the oversize file check, but I think it's good enough to catch serious regressions. --- vello_tests/snapshots/longpathdash_butt.png | 3 +++ vello_tests/tests/snapshot_test_scenes.rs | 8 ++++++++ 2 files changed, 11 insertions(+) create mode 100644 vello_tests/snapshots/longpathdash_butt.png diff --git a/vello_tests/snapshots/longpathdash_butt.png b/vello_tests/snapshots/longpathdash_butt.png new file mode 100644 index 000000000..2ea66108c --- /dev/null +++ b/vello_tests/snapshots/longpathdash_butt.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:f5f52e9cdd7670b3fedf708d5b6eb2e9f42eadba9b822c091ac57e171b178ab1 +size 91104 diff --git a/vello_tests/tests/snapshot_test_scenes.rs b/vello_tests/tests/snapshot_test_scenes.rs index 5f447acd7..12f78e843 100644 --- a/vello_tests/tests/snapshot_test_scenes.rs +++ b/vello_tests/tests/snapshot_test_scenes.rs @@ -97,3 +97,11 @@ fn snapshot_blurred_rounded_rect() { let params = TestParams::new("blurred_rounded_rect", 400, 400); snapshot_test_scene(test_scene, params); } + +#[test] +#[cfg_attr(skip_gpu_tests, ignore)] +fn snapshot_longpathdash_butt() { + let test_scene = test_scenes::longpathdash_butt(); + let params = TestParams::new("longpathdash_butt", 250, 250); + snapshot_test_scene(test_scene, params); +} From 8853b28e7fed484855a732bc90188f34421b419b Mon Sep 17 00:00:00 2001 From: Raph Levien Date: Wed, 2 Oct 2024 15:03:47 -0700 Subject: [PATCH 4/6] Change longpath test image size to include artifact Also tweak threshold so it catches the failure. --- vello_tests/snapshots/longpathdash_butt.png | 4 ++-- vello_tests/tests/snapshot_test_scenes.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/vello_tests/snapshots/longpathdash_butt.png b/vello_tests/snapshots/longpathdash_butt.png index 2ea66108c..843f98f0d 100644 --- a/vello_tests/snapshots/longpathdash_butt.png +++ b/vello_tests/snapshots/longpathdash_butt.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:f5f52e9cdd7670b3fedf708d5b6eb2e9f42eadba9b822c091ac57e171b178ab1 -size 91104 +oid sha256:93dc92000d81921fe6a45a862db066736592e1ffc008ee3c29794287a68b930b +size 64883 diff --git a/vello_tests/tests/snapshot_test_scenes.rs b/vello_tests/tests/snapshot_test_scenes.rs index 12f78e843..2a7945db0 100644 --- a/vello_tests/tests/snapshot_test_scenes.rs +++ b/vello_tests/tests/snapshot_test_scenes.rs @@ -11,7 +11,7 @@ fn snapshot_test_scene(test_scene: ExampleScene, mut params: TestParams) { let scene = encode_test_scene(test_scene, &mut params); snapshot_test_sync(scene, ¶ms) .unwrap() - .assert_mean_less_than(0.01); + .assert_mean_less_than(0.007); } #[test] @@ -102,6 +102,6 @@ fn snapshot_blurred_rounded_rect() { #[cfg_attr(skip_gpu_tests, ignore)] fn snapshot_longpathdash_butt() { let test_scene = test_scenes::longpathdash_butt(); - let params = TestParams::new("longpathdash_butt", 250, 250); + let params = TestParams::new("longpathdash_butt", 500, 100); snapshot_test_scene(test_scene, params); } From 391b25db7a52210b4141e2217e5e99bcf31e98ca Mon Sep 17 00:00:00 2001 From: Raph Levien Date: Wed, 2 Oct 2024 15:26:42 -0700 Subject: [PATCH 5/6] Additional tweaking to snapshot size and threshold --- vello_tests/snapshots/longpathdash_butt.png | 4 ++-- vello_tests/tests/snapshot_test_scenes.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/vello_tests/snapshots/longpathdash_butt.png b/vello_tests/snapshots/longpathdash_butt.png index 843f98f0d..8a9e729df 100644 --- a/vello_tests/snapshots/longpathdash_butt.png +++ b/vello_tests/snapshots/longpathdash_butt.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:93dc92000d81921fe6a45a862db066736592e1ffc008ee3c29794287a68b930b -size 64883 +oid sha256:a1a23c2e7ab9018ff3517cbc96f64befe6fcf7836cca12c17ba6b9e612fda71f +size 42426 diff --git a/vello_tests/tests/snapshot_test_scenes.rs b/vello_tests/tests/snapshot_test_scenes.rs index 2a7945db0..3d2e97299 100644 --- a/vello_tests/tests/snapshot_test_scenes.rs +++ b/vello_tests/tests/snapshot_test_scenes.rs @@ -11,7 +11,7 @@ fn snapshot_test_scene(test_scene: ExampleScene, mut params: TestParams) { let scene = encode_test_scene(test_scene, &mut params); snapshot_test_sync(scene, ¶ms) .unwrap() - .assert_mean_less_than(0.007); + .assert_mean_less_than(0.0095); } #[test] @@ -102,6 +102,6 @@ fn snapshot_blurred_rounded_rect() { #[cfg_attr(skip_gpu_tests, ignore)] fn snapshot_longpathdash_butt() { let test_scene = test_scenes::longpathdash_butt(); - let params = TestParams::new("longpathdash_butt", 500, 100); + let params = TestParams::new("longpathdash_butt", 440, 80); snapshot_test_scene(test_scene, params); } From bae8049630cbb472335b5afa7671e0a382defc70 Mon Sep 17 00:00:00 2001 From: Raph Levien Date: Thu, 3 Oct 2024 07:47:37 -0700 Subject: [PATCH 6/6] Update vello_encoding/src/path.rs Improve docstring Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> --- vello_encoding/src/path.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vello_encoding/src/path.rs b/vello_encoding/src/path.rs index be1baae16..783ed8b44 100644 --- a/vello_encoding/src/path.rs +++ b/vello_encoding/src/path.rs @@ -769,7 +769,7 @@ impl<'a> PathEncoder<'a> { Some(pt) } - // Similar to `start_tangent_for_curve` but for a line. + /// Similar to [`Self::start_tangent_for_curve`] but for a line. fn start_tangent_for_line(&self, p1: (f32, f32)) -> Option<(f32, f32)> { let p0 = (self.first_point[0], self.first_point[1]); let pt = if (p1.0 - p0.0).abs() > EPSILON || (p1.1 - p0.1).abs() > EPSILON {