From c3b462663f7d45d2d1fc91374a68856b484a9a29 Mon Sep 17 00:00:00 2001 From: Jeff Muizelaar Date: Wed, 30 Aug 2023 01:42:23 +0000 Subject: [PATCH] Bug 1849487. Futher improve miter joining. r=lsalzman This avoids numerical stability problems when computing the miter pointer intersection when the lines are nearly parallel. Differential Revision: https://phabricator.services.mozilla.com/D187057 --- .cargo/config.in | 4 +- Cargo.lock | 2 +- .../rust/aa-stroke/.cargo-checksum.json | 2 +- .../rust/aa-stroke/.github/workflows/rust.yml | 12 ++++ third_party/rust/aa-stroke/src/c_bindings.rs | 27 ++++++++- third_party/rust/aa-stroke/src/lib.rs | 60 ++++++++++++++++--- toolkit/library/rust/shared/Cargo.toml | 2 +- 7 files changed, 92 insertions(+), 17 deletions(-) diff --git a/.cargo/config.in b/.cargo/config.in index 9d7780c7f09f..e01bde536d68 100644 --- a/.cargo/config.in +++ b/.cargo/config.in @@ -5,9 +5,9 @@ [source.crates-io] replace-with = "vendored-sources" -[source."git+https://github.com/FirefoxGraphics/aa-stroke?rev=c7bc7190f6d3115bc81640c0433649c1fce9491c"] +[source."git+https://github.com/FirefoxGraphics/aa-stroke?rev=ed4206ea11703580cd1d4fc63371a527b29d8252"] git = "https://github.com/FirefoxGraphics/aa-stroke" -rev = "c7bc7190f6d3115bc81640c0433649c1fce9491c" +rev = "ed4206ea11703580cd1d4fc63371a527b29d8252" replace-with = "vendored-sources" [source."git+https://github.com/FirefoxGraphics/wpf-gpu-raster?rev=5ab6fe33d00021325ee920b3c10526dc8301cf46"] diff --git a/Cargo.lock b/Cargo.lock index b1c2c809d379..f6b2567470ea 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5,7 +5,7 @@ version = 3 [[package]] name = "aa-stroke" version = "0.1.0" -source = "git+https://github.com/FirefoxGraphics/aa-stroke?rev=c7bc7190f6d3115bc81640c0433649c1fce9491c#c7bc7190f6d3115bc81640c0433649c1fce9491c" +source = "git+https://github.com/FirefoxGraphics/aa-stroke?rev=ed4206ea11703580cd1d4fc63371a527b29d8252#ed4206ea11703580cd1d4fc63371a527b29d8252" dependencies = [ "euclid", ] diff --git a/third_party/rust/aa-stroke/.cargo-checksum.json b/third_party/rust/aa-stroke/.cargo-checksum.json index 8941a70c33cd..c1ad1a16e46f 100644 --- a/third_party/rust/aa-stroke/.cargo-checksum.json +++ b/third_party/rust/aa-stroke/.cargo-checksum.json @@ -1 +1 @@ -{"files":{".github/workflows/rust.yml":"e859b12cfed145b66e4fbf1571dde15880359717ca5b0a9720341229183f2c6f","Cargo.toml":"f507cac11c3c26af28420d68ec3748a5453322d51ef1379a340fdd3b1c9b187a","README.md":"60b34cfa653114d5054009696df2ed2ea1d4926a6bc312d0cac4b84845c2beff","examples/simple.rs":"c196e79568fe4be31a08374aa451c70c9377db5428aef924a985e069c12ed91e","src/bezierflattener.rs":"61687da22490cb1bd901d0b5eb1de3a98802b46c03719ded4163c7a4997f0ad9","src/c_bindings.rs":"23890f7bb5d40d87d72b28842821ada7b2e1509a3b37c9af8ed0055036ceadcc","src/lib.rs":"18480fd1f92519229d2439c226011df88a3abf9a6a12295685611cf2bddc8965","src/tri_rasterize.rs":"fb6f595ab9340d8ea6429b41638c378bbd772c8e4d8f7793e225624c12cd3a21"},"package":null} \ No newline at end of file +{"files":{".github/workflows/rust.yml":"6a9f1b122ea02367a2f1ff1fc7b9a728284ceb47fad12e1610cde9d760f4efc3","Cargo.toml":"f507cac11c3c26af28420d68ec3748a5453322d51ef1379a340fdd3b1c9b187a","README.md":"60b34cfa653114d5054009696df2ed2ea1d4926a6bc312d0cac4b84845c2beff","examples/simple.rs":"c196e79568fe4be31a08374aa451c70c9377db5428aef924a985e069c12ed91e","src/bezierflattener.rs":"61687da22490cb1bd901d0b5eb1de3a98802b46c03719ded4163c7a4997f0ad9","src/c_bindings.rs":"7cc3916665a89b8dc1eb1d1ddd2c966e6ece06dba04d228a8ebd807fb2270345","src/lib.rs":"9719ba050cc0ff6093c60646321f0cbedd06d6483106119bd9ca05d6e42ddaae","src/tri_rasterize.rs":"fb6f595ab9340d8ea6429b41638c378bbd772c8e4d8f7793e225624c12cd3a21"},"package":null} \ No newline at end of file diff --git a/third_party/rust/aa-stroke/.github/workflows/rust.yml b/third_party/rust/aa-stroke/.github/workflows/rust.yml index e78f9416c94b..a76ca20483e8 100644 --- a/third_party/rust/aa-stroke/.github/workflows/rust.yml +++ b/third_party/rust/aa-stroke/.github/workflows/rust.yml @@ -18,3 +18,15 @@ jobs: run: cargo build --verbose --features=c_bindings - name: Run tests run: cargo test --verbose --features=c_bindings + miri: + name: "Miri" + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - name: Install Miri + run: | + rustup toolchain install nightly --component miri + rustup override set nightly + cargo miri setup + - name: Test with Miri + run: cargo miri test --lib --bins --tests --examples diff --git a/third_party/rust/aa-stroke/src/c_bindings.rs b/third_party/rust/aa-stroke/src/c_bindings.rs index 8880e492938b..455568aebdec 100644 --- a/third_party/rust/aa-stroke/src/c_bindings.rs +++ b/third_party/rust/aa-stroke/src/c_bindings.rs @@ -60,14 +60,17 @@ pub extern "C" fn aa_stroke_close(s: &mut Stroker) { pub extern "C" fn aa_stroke_finish(s: &mut Stroker) -> VertexBuffer { let stroked_path = s.get_stroked_path(); if let Some(output_buffer_size) = stroked_path.get_output_buffer_size() { + // if let Some(output_buffer) = stroked_path.output_buffer { + // dbg!(&output_buffer[0..output_buffer_size]); + // } VertexBuffer { data: std::ptr::null(), len: output_buffer_size, } } else { - let result = stroked_path.finish(); - let vb = VertexBuffer { data: result.as_ptr(), len: result.len() }; - std::mem::forget(result); + let result = s.finish(); + let len = result.len(); + let vb = VertexBuffer { data: Box::leak(result).as_ptr(), len }; vb } } @@ -99,3 +102,21 @@ fn simple() { aa_stroke_vertex_buffer_release(vb); unsafe { aa_stroke_release(s) } ; } + +#[test] +fn output_buffer() { + let style = StrokeStyle::default(); + let mut output = Vec::new(); + output.resize_with(1000, || OutputVertex{x: 0., y: 0., coverage: 0.}); + let s = unsafe { &mut *aa_stroke_new(&style, output.as_mut_ptr(), output.len()) } ; + aa_stroke_move_to(s, 10., 10., false); + aa_stroke_line_to(s, 100., 100., false); + aa_stroke_line_to(s, 100., 10., true); + + let vb = aa_stroke_finish(s); + assert_ne!(vb.len, 0); + assert_eq!(vb.data, std::ptr::null()); + aa_stroke_vertex_buffer_release(vb); + unsafe { aa_stroke_release(s) } ; +} + diff --git a/third_party/rust/aa-stroke/src/lib.rs b/third_party/rust/aa-stroke/src/lib.rs index a9802fb6e03a..b168e0f3b376 100644 --- a/third_party/rust/aa-stroke/src/lib.rs +++ b/third_party/rust/aa-stroke/src/lib.rs @@ -156,8 +156,8 @@ impl<'z> PathBuilder<'z> { } /// Completes the current path - pub fn finish(self) -> Vec { - self.vertices + pub fn finish(self) -> Box<[Vertex]> { + self.vertices.into_boxed_slice() } pub fn get_output_buffer_size(&self) -> Option { @@ -620,10 +620,14 @@ fn join_line( let mid = bisect(s1_normal, s2_normal); // cross = sin, dot = cos - let s = cross(mid, s1_normal)/(1. + dot(s1_normal, mid)); + let cos = dot(s1_normal, mid); + let s = cross(mid, s1_normal)/(1. + cos); + + // compute the intersection in a more stable way + let intersection = pt + mid * (offset / cos); let ramp_s1 = intersection + s1_normal * 1. + unperp(s1_normal) * s; - let ramp_s2 = intersection + s2_normal * 1. + unperp(s2_normal) * s; + let ramp_s2 = intersection + s2_normal * 1. + perp(s2_normal) * s; dest.ramp(intersection.x, intersection.y, ramp_s1.x, ramp_s1.y, @@ -707,12 +711,16 @@ impl<'z> Stroker<'z> { } pub fn move_to(&mut self, pt: Point, closed_subpath: bool) { + //eprintln!("stroker.move_to(Point::new({}, {}), {});", pt.x, pt.y, closed_subpath); + self.start_point = None; self.cur_pt = Some(pt); self.closed_subpath = closed_subpath; } pub fn line_to(&mut self, pt: Point) { + //eprintln!("stroker.line_to(Point::new({}, {}));", pt.x, pt.y); + let cur_pt = self.cur_pt; let stroked_path = &mut self.stroked_path; let half_width = self.half_width; @@ -784,6 +792,7 @@ impl<'z> Stroker<'z> { } pub fn curve_to(&mut self, cx1: Point, cx2: Point, pt: Point) { + //eprintln!("stroker.curve_to(Point::new({}, {}), Point::new({}, {}), Point::new({}, {}));", cx1.x, cx1.y, cx2.x, cx2.y, pt.x, pt.y); self.curve_to_internal(cx1, cx2, pt, false); } @@ -890,7 +899,7 @@ impl<'z> Stroker<'z> { stroked_path } - pub fn finish(&mut self) -> Vec { + pub fn finish(&mut self) -> Box<[Vertex]> { self.get_stroked_path().finish() } } @@ -939,7 +948,7 @@ fn butt_cap() { stroker.move_to(Point::new(20., 20.5), false); stroker.line_to_capped(Point::new(40., 20.5)); let result = stroker.finish(); - for v in result { + for v in result.iter() { assert!(v.y == 20.5 || v.y == 19.5 || v.y == 21.5); } } @@ -1005,7 +1014,7 @@ fn degenerate_miter_join() { let result = stroker.finish(); // make sure none of the verticies are wildly out of place - for v in result { + for v in result.iter() { assert!(v.y >= 527.); } @@ -1025,8 +1034,41 @@ fn degenerate_miter_join() { stroker.line_to(Point::new(512.3874, 598.6111)); stroker.line_to_capped(end); let result = stroker.finish(); - for v in result { - assert!(dbg!(distance_from_line(start, end, Point::new(v.x, v.y))) <= 21.); + for v in result.iter() { + assert!(distance_from_line(start, end, Point::new(v.x, v.y)) <= 21.); } + + // from https://stackoverflow.com/questions/849211/shortest-distance-between-a-point-and-a-line-segment + fn minimum_distance(v: Point, w: Point, p: Point) -> f32 { + // Return minimum distance between line segment vw and point p + let l2 = (v-w).length().powi(2); // i.e. |w-v|^2 - avoid a sqrt + if l2 == 0.0 { return (p - v).length(); } // v == w case + // Consider the line extending the segment, parameterized as v + t (w - v). + // We find projection of point p onto the line. + // It falls where t = [(p-v) . (w-v)] / |w-v|^2 + // We clamp t from [0,1] to handle points outside the segment vw. + let t = 0_f32.max(1_f32.min(dot(p - v, w - v) / l2)); + let projection = v + (w - v) * t; // Projection falls on the segment + (p - projection).length() + } + + let mut stroker = Stroker::new(&StrokeStyle{ + cap: LineCap::Square, + join: LineJoin::Miter, + width: 40.0, + miter_limit: 10.0, + ..Default::default()}); + let start = Point::new(689.3504, 434.5446); + let end = Point::new(671.83203, 422.61914); + stroker.move_to(Point::new(689.3504, 434.5446), false); + stroker.line_to(Point::new(681.04254, 428.8891)); + stroker.line_to_capped(Point::new(671.83203, 422.61914)); + + let result = stroker.finish(); + let max_distance = (21_f32.powi(2) * 2.).sqrt(); + for v in result.iter() { + assert!(minimum_distance(start, end, Point::new(v.x, v.y)) <= max_distance); + } + } diff --git a/toolkit/library/rust/shared/Cargo.toml b/toolkit/library/rust/shared/Cargo.toml index 3b347d653fac..a30fee8fa394 100644 --- a/toolkit/library/rust/shared/Cargo.toml +++ b/toolkit/library/rust/shared/Cargo.toml @@ -95,7 +95,7 @@ processtools = { path = "../../../components/processtools" } qcms = { path = "../../../../gfx/qcms", features = ["c_bindings", "neon"], default-features = false } wpf-gpu-raster = { git = "https://github.com/FirefoxGraphics/wpf-gpu-raster", rev = "5ab6fe33d00021325ee920b3c10526dc8301cf46" } -aa-stroke = { git = "https://github.com/FirefoxGraphics/aa-stroke", rev = "c7bc7190f6d3115bc81640c0433649c1fce9491c" } +aa-stroke = { git = "https://github.com/FirefoxGraphics/aa-stroke", rev = "ed4206ea11703580cd1d4fc63371a527b29d8252" } # Force url to stay at 2.1.0. See bug 1734538. url = "=2.1.0"