From 6dd71dda8716522e01538cf9ea06352077afc009 Mon Sep 17 00:00:00 2001 From: Raph Levien Date: Tue, 31 Oct 2023 17:06:25 -0700 Subject: [PATCH 1/7] Add test scene to trigger numerical robustness issues This test scene contains a number of subpaths that contain line segments aligned to the grid, or that cross grid corners. These trigger one-pixel artifacts with the multisampled rendering modes, and are now correct with area antialiasing. --- examples/scenes/src/test_scenes.rs | 53 ++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/examples/scenes/src/test_scenes.rs b/examples/scenes/src/test_scenes.rs index 75678d80f..cddab2ace 100644 --- a/examples/scenes/src/test_scenes.rs +++ b/examples/scenes/src/test_scenes.rs @@ -44,6 +44,7 @@ pub fn test_scenes() -> SceneSet { scene!(blend_grid), scene!(conflation_artifacts), scene!(labyrinth), + scene!(robust_paths), scene!(base_color_test: animated), scene!(clip_test: animated), scene!(longpathdash(Cap::Butt), "longpathdash (butt caps)", false), @@ -1119,6 +1120,58 @@ fn labyrinth(sb: &mut SceneBuilder, _: &mut SceneParams) { ) } +fn robust_paths(sb: &mut SceneBuilder, _: &mut SceneParams) { + let mut path = BezPath::new(); + path.move_to((16.0, 16.0)); + path.line_to((32.0, 16.0)); + path.line_to((32.0, 32.0)); + path.line_to((16.0, 32.0)); + path.close_path(); + path.move_to((48.0, 18.0)); + path.line_to((64.0, 23.0)); + path.line_to((64.0, 33.0)); + path.line_to((48.0, 38.0)); + path.close_path(); + path.move_to((80.0, 18.0)); + path.line_to((82.0, 16.0)); + path.line_to((94.0, 16.0)); + path.line_to((96.0, 18.0)); + path.line_to((96.0, 30.0)); + path.line_to((94.0, 32.0)); + path.line_to((82.0, 32.0)); + path.line_to((80.0, 30.0)); + path.close_path(); + path.move_to((112.0, 16.0)); + path.line_to((128.0, 16.0)); + path.line_to((128.0, 32.0)); + path.close_path(); + path.move_to((144.0, 16.0)); + path.line_to((160.0, 32.0)); + path.line_to((144.0, 32.0)); + path.close_path(); + path.move_to((168.0, 8.0)); + path.line_to((184.0, 8.0)); + path.line_to((184.0, 24.0)); + path.close_path(); + path.move_to((200.0, 8.0)); + path.line_to((216.0, 24.0)); + path.line_to((200.0, 24.0)); + path.close_path(); + sb.fill(Fill::NonZero, Affine::IDENTITY, Color::YELLOW, None, &path); + path.move_to((8.0, 4.0)); + path.line_to((8.0, 40.0)); + path.line_to((220.0, 40.0)); + path.line_to((220.0, 4.0)); + path.close_path(); + sb.fill( + Fill::NonZero, + Affine::translate((0.0, 100.0)), + Color::YELLOW, + None, + &path, + ); +} + fn base_color_test(sb: &mut SceneBuilder, params: &mut SceneParams) { // Cycle through the hue value every 5 seconds (t % 5) * 360/5 let color = Color::hlc((params.time % 5.0) * 72.0, 80.0, 80.0); From faf288aeabbcc9cf0bb93c971eefd2c8b222f46c Mon Sep 17 00:00:00 2001 From: Raph Levien Date: Mon, 30 Oct 2023 10:50:46 -0700 Subject: [PATCH 2/7] Change to tile-relative segment coordinates WIP. Currently works but no cleanup. --- shader/fine.wgsl | 27 ++++++++++++--------------- shader/path_tiling.wgsl | 2 +- src/cpu_shader/path_tiling.rs | 4 ++-- 3 files changed, 15 insertions(+), 18 deletions(-) diff --git a/shader/fine.wgsl b/shader/fine.wgsl index 5d537f6a5..cc5c1c8e3 100644 --- a/shader/fine.wgsl +++ b/shader/fine.wgsl @@ -130,16 +130,15 @@ let ROBUST_EPSILON: f32 = 2e-7; // is invited to study the even-odd case first, as there only one bit is // needed to represent a winding number parity, thus there is a lot less // bit shifting, and less shuffling altogether. -fn fill_path_ms(fill: CmdFill, wg_id: vec2, local_id: vec2, result: ptr>) { +fn fill_path_ms(fill: CmdFill, local_id: vec2, result: ptr>) { let even_odd = (fill.size_and_rule & 1u) != 0u; // This isn't a divergent branch because the fill parameters are workgroup uniform, // provably so because the ptcl buffer is bound read-only. if even_odd { - fill_path_ms_evenodd(fill, wg_id, local_id, result); + fill_path_ms_evenodd(fill, local_id, result); return; } let n_segs = fill.size_and_rule >> 1u; - let tile_origin = vec2(f32(wg_id.x) * f32(TILE_HEIGHT), f32(wg_id.y) * f32(TILE_WIDTH)); let th_ix = local_id.y * (TILE_WIDTH / PIXELS_PER_THREAD) + local_id.x; // Initialize winding number arrays to a winding number of 0, which is 0x80 in an // 8 bit biased signed integer encoding. @@ -163,9 +162,7 @@ fn fill_path_ms(fill: CmdFill, wg_id: vec2, local_id: vec2, result: pt // TODO: might save a register rewriting this in terms of limit if th_ix < slice_size { let segment = segments[seg_off]; - // Note: coords relative to tile origin probably a good idea in coarse path, - // especially as f16 would work. But keeping existing scheme for compatibility. - let xy0 = segment.origin - tile_origin; + let xy0 = segment.origin; let xy1 = xy0 + segment.delta; var y_edge_f = f32(TILE_HEIGHT); var delta = select(-1, 1, xy1.x <= xy0.x); @@ -224,7 +221,8 @@ fn fill_path_ms(fill: CmdFill, wg_id: vec2, local_id: vec2, result: pt let sub_ix = i - select(0u, sh_count[el_ix - 1u], el_ix > 0u); let seg_off = fill.seg_data + batch * WG_SIZE + el_ix; let segment = segments[seg_off]; - let xy0_in = segment.origin - tile_origin; + // Coordinates are relative to tile origin + let xy0_in = segment.origin; let xy1_in = xy0_in + segment.delta; let is_down = xy1_in.y >= xy0_in.y; let xy0 = select(xy1_in, xy0_in, is_down); @@ -492,9 +490,8 @@ fn fill_path_ms(fill: CmdFill, wg_id: vec2, local_id: vec2, result: pt // as both have the same effect on winding number. // // TODO: factor some logic out to reduce code duplication. -fn fill_path_ms_evenodd(fill: CmdFill, wg_id: vec2, local_id: vec2, result: ptr>) { +fn fill_path_ms_evenodd(fill: CmdFill, local_id: vec2, result: ptr>) { let n_segs = fill.size_and_rule >> 1u; - let tile_origin = vec2(f32(wg_id.x) * f32(TILE_HEIGHT), f32(wg_id.y) * f32(TILE_WIDTH)); let th_ix = local_id.y * (TILE_WIDTH / PIXELS_PER_THREAD) + local_id.x; if th_ix < TILE_HEIGHT { if th_ix == 0u { @@ -516,9 +513,8 @@ fn fill_path_ms_evenodd(fill: CmdFill, wg_id: vec2, local_id: vec2, re // TODO: might save a register rewriting this in terms of limit if th_ix < slice_size { let segment = segments[seg_off]; - // Note: coords relative to tile origin probably a good idea in coarse path, - // especially as f16 would work. But keeping existing scheme for compatibility. - let xy0 = segment.origin - tile_origin; + // Coordinates are relative to tile origin + let xy0 = segment.origin; let xy1 = xy0 + segment.delta; var y_edge_f = f32(TILE_HEIGHT); if xy0.x == 0.0 && xy1.x == 0.0 { @@ -575,7 +571,7 @@ fn fill_path_ms_evenodd(fill: CmdFill, wg_id: vec2, local_id: vec2, re let sub_ix = i - select(0u, sh_count[el_ix - 1u], el_ix > 0u); let seg_off = fill.seg_data + batch * WG_SIZE + el_ix; let segment = segments[seg_off]; - let xy0_in = segment.origin - tile_origin; + let xy0_in = segment.origin; let xy1_in = xy0_in + segment.delta; let is_down = xy1_in.y >= xy0_in.y; let xy0 = select(xy1_in, xy0_in, is_down); @@ -864,6 +860,7 @@ fn main( ) { let tile_ix = wg_id.y * config.width_in_tiles + wg_id.x; let xy = vec2(f32(global_id.x * PIXELS_PER_THREAD), f32(global_id.y)); + let local_xy = vec2(f32(local_id.x * PIXELS_PER_THREAD), f32(local_id.y)); #ifdef full var rgba: array, PIXELS_PER_THREAD>; for (var i = 0u; i < PIXELS_PER_THREAD; i += 1u) { @@ -886,9 +883,9 @@ fn main( case 1u: { let fill = read_fill(cmd_ix); #ifdef msaa - fill_path_ms(fill, wg_id.xy, local_id.xy, &area); + fill_path_ms(fill, local_id.xy, &area); #else - fill_path(fill, xy, &area); + fill_path(fill, local_xy, &area); #endif cmd_ix += 4u; } diff --git a/shader/path_tiling.wgsl b/shader/path_tiling.wgsl index abfa4475d..04a640d02 100644 --- a/shader/path_tiling.wgsl +++ b/shader/path_tiling.wgsl @@ -132,7 +132,7 @@ fn main( xy0 = xy1; xy1 = tmp; } - let segment = Segment(xy0, xy1 - xy0, y_edge); + let segment = Segment(xy0 - tile_xy, xy1 - xy0, y_edge - tile_xy.y); segments[seg_start + seg_within_slice] = segment; } } diff --git a/src/cpu_shader/path_tiling.rs b/src/cpu_shader/path_tiling.rs index 6b8a9413e..bf2975233 100644 --- a/src/cpu_shader/path_tiling.rs +++ b/src/cpu_shader/path_tiling.rs @@ -128,9 +128,9 @@ fn path_tiling_main( 1e9 }; let segment = PathSegment { - origin: xy0.to_array(), + origin: (xy0 - tile_xy).to_array(), delta: (xy1 - xy0).to_array(), - y_edge, + y_edge: y_edge - tile_xy.y, _padding: Default::default(), }; assert!(xy0.x >= tile_xy.x && xy0.x <= tile_xy1.x); From 6f2a83e99057227f0e4409af238fdec3b3ab6add Mon Sep 17 00:00:00 2001 From: Raph Levien Date: Mon, 30 Oct 2023 11:58:01 -0700 Subject: [PATCH 3/7] Move numerical robustness into tiling Change representation to tile-relative coordinates for segment endpoints. --- crates/encoding/src/path.rs | 4 +- shader/fine.wgsl | 84 +++++++++++++---------------------- shader/path_tiling.wgsl | 41 +++++++++++++---- shader/shared/segment.wgsl | 5 ++- src/cpu_shader/fine.rs | 20 +++++---- src/cpu_shader/path_tiling.rs | 54 +++++++++++++++------- 6 files changed, 118 insertions(+), 90 deletions(-) diff --git a/crates/encoding/src/path.rs b/crates/encoding/src/path.rs index 83e537042..c1c521d30 100644 --- a/crates/encoding/src/path.rs +++ b/crates/encoding/src/path.rs @@ -207,8 +207,8 @@ pub struct SegmentCount { #[derive(Clone, Copy, Debug, Zeroable, Pod, Default)] #[repr(C)] pub struct PathSegment { - pub origin: [f32; 2], - pub delta: [f32; 2], + pub point0: [f32; 2], + pub point1: [f32; 2], pub y_edge: f32, pub _padding: u32, } diff --git a/shader/fine.wgsl b/shader/fine.wgsl index cc5c1c8e3..016095327 100644 --- a/shader/fine.wgsl +++ b/shader/fine.wgsl @@ -162,29 +162,18 @@ fn fill_path_ms(fill: CmdFill, local_id: vec2, result: ptr, result: ptr= xy0_in.y; let xy0 = select(xy1_in, xy0_in, is_down); let xy1 = select(xy0_in, xy1_in, is_down); @@ -514,27 +503,17 @@ fn fill_path_ms_evenodd(fill: CmdFill, local_id: vec2, result: ptr, result: ptr 0u); let seg_off = fill.seg_data + batch * WG_SIZE + el_ix; let segment = segments[seg_off]; - let xy0_in = segment.origin; - let xy1_in = xy0_in + segment.delta; + let xy0_in = segment.point0; + let xy1_in = segment.point1; let is_down = xy1_in.y >= xy0_in.y; let xy0 = select(xy1_in, xy0_in, is_down); let xy1 = select(xy0_in, xy1_in, is_down); @@ -807,17 +786,18 @@ fn fill_path(fill: CmdFill, xy: vec2, result: ptr, result: ptr, - delta: vec2, + // Points are relative to tile origin + point0: vec2, + point1: vec2, y_edge: f32, } diff --git a/src/cpu_shader/fine.rs b/src/cpu_shader/fine.rs index c64c87627..dc3203959 100644 --- a/src/cpu_shader/fine.rs +++ b/src/cpu_shader/fine.rs @@ -58,20 +58,24 @@ fn fill_path(area: &mut [f32], segments: &[PathSegment], fill: &CmdFill, x_tile: *a = backdrop_f; } for segment in &segments[fill.seg_data as usize..][..n_segs as usize] { + let delta = [ + segment.point1[0] - segment.point0[0], + segment.point1[1] - segment.point0[1], + ]; for yi in 0..TILE_HEIGHT { - let y = segment.origin[1] - (y_tile + yi as f32); + let y = segment.point0[1] - (y_tile + yi as f32); let y0 = y.clamp(0.0, 1.0); - let y1 = (y + segment.delta[1]).clamp(0.0, 1.0); + let y1 = (y + delta[1]).clamp(0.0, 1.0); let dy = y0 - y1; - let y_edge = segment.delta[0].signum() - * (y_tile + yi as f32 - segment.y_edge + 1.0).clamp(0.0, 1.0); + let y_edge = + delta[0].signum() * (y_tile + yi as f32 - segment.y_edge + 1.0).clamp(0.0, 1.0); if dy != 0.0 { - let vec_y_recip = segment.delta[1].recip(); + let vec_y_recip = delta[1].recip(); let t0 = (y0 - y) * vec_y_recip; let t1 = (y1 - y) * vec_y_recip; - let startx = segment.origin[0] - x_tile; - let x0 = startx + t0 * segment.delta[0]; - let x1 = startx + t1 * segment.delta[0]; + let startx = segment.point0[0] - x_tile; + let x0 = startx + t0 * delta[0]; + let x1 = startx + t1 * delta[0]; let xmin0 = x0.min(x1); let xmax0 = x0.max(x1); for i in 0..TILE_WIDTH { diff --git a/src/cpu_shader/path_tiling.rs b/src/cpu_shader/path_tiling.rs index bf2975233..849474cf9 100644 --- a/src/cpu_shader/path_tiling.rs +++ b/src/cpu_shader/path_tiling.rs @@ -115,28 +115,48 @@ fn path_tiling_main( xy1 = Vec2::new(x_clip, yt); } } + let mut y_edge = 1e9; + // Apply numerical robustness logic + let mut p0 = xy0 - tile_xy; + let mut p1 = xy1 - tile_xy; + const EPSILON: f32 = 1e-6; + if p0.x == 0.0 { + if p1.x == 0.0 { + p0.x = EPSILON; + if p0.y == 0.0 { + // Entire tile + p1.x = EPSILON; + p1.y = TILE_HEIGHT as f32; + } else { + // Make segment disappear + p1.x = 2.0 * EPSILON; + p1.y = p0.y; + } + } else if p0.y == 0.0 { + p0.x = EPSILON; + } else { + y_edge = p0.y; + } + } else if p1.x == 0.0 { + if p1.y == 0.0 { + p1.x = EPSILON; + } else { + y_edge = p1.y; + } + } if !is_down { - (xy0, xy1) = (xy1, xy0); + (p0, p1) = (p1, p0); } - // TODO (part of move to 8 byte encoding for segments): don't store y_edge at all, - // resolve this in fine. - let y_edge = if xy0.x == tile_xy.x && xy1.x != tile_xy.x && xy0.y != tile_xy.y { - xy0.y - } else if xy1.x == tile_xy.x && xy1.y != tile_xy.y { - xy1.y - } else { - 1e9 - }; let segment = PathSegment { - origin: (xy0 - tile_xy).to_array(), - delta: (xy1 - xy0).to_array(), - y_edge: y_edge - tile_xy.y, + point0: p0.to_array(), + point1: p1.to_array(), + y_edge, _padding: Default::default(), }; - assert!(xy0.x >= tile_xy.x && xy0.x <= tile_xy1.x); - assert!(xy0.y >= tile_xy.y && xy0.y <= tile_xy1.y); - assert!(xy1.x >= tile_xy.x && xy1.x <= tile_xy1.x); - assert!(xy1.y >= tile_xy.y && xy1.y <= tile_xy1.y); + assert!(p0.x >= 0.0 && p0.x <= TILE_WIDTH as f32); + assert!(p0.y >= 0.0 && p0.y <= TILE_HEIGHT as f32); + assert!(p1.x >= 0.0 && p1.x <= TILE_WIDTH as f32); + assert!(p1.y >= 0.0 && p1.y <= TILE_HEIGHT as f32); segments[(seg_start + seg_within_slice) as usize] = segment; } } From b6e90a9bf71c15ea1b0ba29b329e568089dd7721 Mon Sep 17 00:00:00 2001 From: Raph Levien Date: Wed, 1 Nov 2023 10:57:43 -0700 Subject: [PATCH 4/7] Test code for msaa --- shader/fine.wgsl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/shader/fine.wgsl b/shader/fine.wgsl index 016095327..7fc2352cd 100644 --- a/shader/fine.wgsl +++ b/shader/fine.wgsl @@ -452,7 +452,7 @@ fn fill_path_ms(fill: CmdFill, local_id: vec2, result: ptr> 1u); // xored23 contains 2-reductions from words 2 and 3, interleaved @@ -590,7 +590,7 @@ fn fill_path_ms_evenodd(fill: CmdFill, local_id: vec2, result: ptr Date: Wed, 1 Nov 2023 13:17:27 -0700 Subject: [PATCH 5/7] Antialiasing fixes This commit fixes a number of problems with multisampled AA. There are three main changes: * This fixes a cut'n'paste typo where the third word of four in resolution was computed incorrectly. This degraded antialiasing quality. * There is a bit more logic in `is_bump` to handle crossings at the top of the tile better. This is not done in the even-odd variant because it is seemingly not needed. (I don't have a careful analysis why, but suspect it has something to do with the fact that double counting or getting the sign wrong is less critical in even-odd than nonzero). * There is a tweak in tiling to cause vertical lines sent to fine to be not pixel-aligned. That's a difficult case for msaa (area doesn't care). The latter two might be considered hacky, but examination of the test cases should improve confidence. There were artifacts in glyph rendering that this patch also improves, so in any case it's better than the previous state. --- examples/scenes/src/test_scenes.rs | 29 +++++++++++++++++++++++++++-- shader/fine.wgsl | 4 ++-- shader/path_tiling.wgsl | 11 +++++++++++ src/cpu_shader/path_tiling.rs | 6 ++++++ 4 files changed, 46 insertions(+), 4 deletions(-) diff --git a/examples/scenes/src/test_scenes.rs b/examples/scenes/src/test_scenes.rs index cddab2ace..8fcf9541c 100644 --- a/examples/scenes/src/test_scenes.rs +++ b/examples/scenes/src/test_scenes.rs @@ -1157,11 +1157,29 @@ fn robust_paths(sb: &mut SceneBuilder, _: &mut SceneParams) { path.line_to((216.0, 24.0)); path.line_to((200.0, 24.0)); path.close_path(); + path.move_to((241.0, 17.5)); + path.line_to((255.0, 17.5)); + path.line_to((255.0, 19.5)); + path.line_to((241.0, 19.5)); + path.close_path(); + path.move_to((241.0, 22.5)); + path.line_to((256.0, 22.5)); + path.line_to((256.0, 24.5)); + path.line_to((241.0, 24.5)); + path.close_path(); sb.fill(Fill::NonZero, Affine::IDENTITY, Color::YELLOW, None, &path); + sb.fill( + Fill::EvenOdd, + Affine::translate((300.0, 0.0)), + Color::LIME, + None, + &path, + ); + path.move_to((8.0, 4.0)); path.line_to((8.0, 40.0)); - path.line_to((220.0, 40.0)); - path.line_to((220.0, 4.0)); + path.line_to((260.0, 40.0)); + path.line_to((260.0, 4.0)); path.close_path(); sb.fill( Fill::NonZero, @@ -1170,6 +1188,13 @@ fn robust_paths(sb: &mut SceneBuilder, _: &mut SceneParams) { None, &path, ); + sb.fill( + Fill::EvenOdd, + Affine::translate((300.0, 100.0)), + Color::LIME, + None, + &path, + ); } fn base_color_test(sb: &mut SceneBuilder, params: &mut SceneParams) { diff --git a/shader/fine.wgsl b/shader/fine.wgsl index 7fc2352cd..8e3046f82 100644 --- a/shader/fine.wgsl +++ b/shader/fine.wgsl @@ -251,7 +251,7 @@ fn fill_path_ms(fill: CmdFill, local_id: vec2, result: ptr, result: ptr Date: Wed, 1 Nov 2023 13:50:55 -0700 Subject: [PATCH 6/7] Unbreak CI CI is broken because the WebGPU stuff doesn't have stable semver guarantees. This pins the web-sys version to protect against that. --- examples/with_winit/Cargo.toml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/examples/with_winit/Cargo.toml b/examples/with_winit/Cargo.toml index 8b4028a8d..4d3d2e72c 100644 --- a/examples/with_winit/Cargo.toml +++ b/examples/with_winit/Cargo.toml @@ -46,5 +46,7 @@ android_logger = "0.13.0" console_error_panic_hook = "0.1.7" console_log = "1" wasm-bindgen-futures = "0.4.33" -web-sys = { version = "0.3.60", features = [ "HtmlCollection", "Text" ] } +# Note: pinning the exact dep here because 0.3.65 broke semver. Update this +# when revving wgpu. +web-sys = { version = "=0.3.64", features = [ "HtmlCollection", "Text" ] } getrandom = { version = "0.2.10", features = ["js"] } From 5a9ddf4e6f6ef9f65841fd84e8f2f8c33bd962d5 Mon Sep 17 00:00:00 2001 From: Raph Levien Date: Wed, 1 Nov 2023 14:57:27 -0700 Subject: [PATCH 7/7] Add comments to help explain msaa logic To say that the msaa logic is dense and hard to understand is an understatement. I've added some comments which will I hope will help the reader navigate the code, but ultimately it requires a real explanation. While reviewing the code, I realized that horizontal pixel-aligned lines are already being discarded by separate robustness logic, so that case doesn't need to be handled in is_bump, so I took it out. --- crates/encoding/src/path.rs | 1 + shader/fine.wgsl | 27 +++++++++++++++++++++------ 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/crates/encoding/src/path.rs b/crates/encoding/src/path.rs index c1c521d30..fb97f2d73 100644 --- a/crates/encoding/src/path.rs +++ b/crates/encoding/src/path.rs @@ -207,6 +207,7 @@ pub struct SegmentCount { #[derive(Clone, Copy, Debug, Zeroable, Pod, Default)] #[repr(C)] pub struct PathSegment { + // Points are relative to tile origin pub point0: [f32; 2], pub point1: [f32; 2], pub y_edge: f32, diff --git a/shader/fine.wgsl b/shader/fine.wgsl index 8e3046f82..80c349487 100644 --- a/shader/fine.wgsl +++ b/shader/fine.wgsl @@ -224,6 +224,8 @@ fn fill_path_ms(fill: CmdFill, local_id: vec2, result: ptr= xy0.x; let x_sign = select(-1.0, 1.0, is_positive_slope); let xt0 = floor(xy0.x * x_sign); @@ -244,15 +246,29 @@ fn fill_path_ms(fill: CmdFill, local_id: vec2, result: ptr, result: ptr