From 8623bdde6cd34d9ee457343239764aaa9b4392a3 Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Wed, 23 Dec 2015 15:59:23 -0800 Subject: [PATCH] Fix a couple of correctness issues around border-radius. 1. Masks for large border radii were not being tessellated properly. This commit simply switches off tessellation for them. Eventually we may want to go back and fix this properly. 2. Border corners were being rendered correctly when 0 < border radius < border width. We needed to do a bit more tessellation than we were. This commit fixes that and adds a diagram to `add_border_corner` to explain what's going on. These commits fix the `border_radius_clip_a.html` and `border_radius_zero_sizes_a.html` reftests. --- src/batch_builder.rs | 261 +++++++++++++++++++++++++++++------------- src/internal_types.rs | 21 +++- src/renderer.rs | 16 ++- src/resource_list.rs | 27 ++--- src/texture_cache.rs | 17 ++- src/util.rs | 7 ++ 6 files changed, 244 insertions(+), 105 deletions(-) diff --git a/src/batch_builder.rs b/src/batch_builder.rs index 35bb68c606..f34e5e3113 100644 --- a/src/batch_builder.rs +++ b/src/batch_builder.rs @@ -918,7 +918,7 @@ impl<'a> BatchBuilder<'a> { BorderRadiusRasterOp::create(&Size2D::new(mask_radius, mask_radius), &Size2D::new(0.0, 0.0), false, - 0, + None, ImageFormat::RGBA8).expect( "Didn't find border radius mask for dashed border!"); let raster_item = RasterItem::BorderRadius(raster_op); @@ -1005,10 +1005,38 @@ impl<'a> BatchBuilder<'a> { } } - #[inline] + /// Draws a border corner. + /// + /// The following diagram attempts to explain the parameters to this function. It's an enlarged + /// version of a border corner that looks like this: + /// + /// ╭─ + /// │ + /// + /// The parameters are as follows: + /// + /// ⤹ corner_bounds.origin + /// ∙┈┈┈┈┈┬┈┈┈┈┈┬┈┈┈┈┈ + /// ┊ ╱ ┊ ┊ + /// ┊ ╱ ┊ ┊ + /// ┊ ╱╲ ┊ ←─┼── color1 + /// ┊╱ ╲ ┊ ┊ + /// ┊ ╲┊ ┊ + /// ├┈┈┈┈┈∙←────┼── radius_extent + /// ┊ ┊╲ ┊ + /// ┊ ┊ ╲ ┊ + /// ┊ ┊ ╲ ┊ + /// ┊ ↑ ┊ ╲ ┊ + /// ┊ │ ┊ ╲┊ + /// ├┈┈┼┈┈┴┈┈┈┈┈∙┈┈┈┈┈ + /// ┊ │ ┊↖︎ + /// ┊ ┊ corner_bounds.bottom_right() + /// ┊color0 ┊ + /// fn add_border_corner(&mut self, clip: &CombinedClipRegion, - vertices_rect: &Rect, + corner_bounds: &Rect, + radius_extent: &Point2D, color0: &ColorF, color1: &ColorF, outer_radius: &Size2D, @@ -1021,25 +1049,47 @@ impl<'a> BatchBuilder<'a> { } // TODO: Check for zero width/height borders! - let white_image = resource_cache.get_dummy_color_image(); + let tl_rect = Rect::new(corner_bounds.origin, + Size2D::new(radius_extent.x - corner_bounds.origin.x, + radius_extent.y - corner_bounds.origin.y)); + let tr_rect = Rect::new(Point2D::new(radius_extent.x, corner_bounds.origin.y), + Size2D::new(corner_bounds.max_x() - radius_extent.x, + radius_extent.y - corner_bounds.origin.y)); + let bl_rect = Rect::new(Point2D::new(corner_bounds.origin.x, radius_extent.y), + Size2D::new(radius_extent.x - corner_bounds.origin.x, + corner_bounds.max_y() - radius_extent.y)); + let br_rect = Rect::new(*radius_extent, + Size2D::new(corner_bounds.max_x() - radius_extent.x, + corner_bounds.max_y() - radius_extent.y)); + + // FIXME(pcwalton): It's kind of messy to be matching on the rotation angle here to pick + // the right rect to draw the rounded corner in. Is there a more elegant way to do this? + let (border_corner_rect, inner_rect, color0_rect, color1_rect) = + match rotation_angle { + BasicRotationAngle::Upright => (&tl_rect, &br_rect, &bl_rect, &tr_rect), + BasicRotationAngle::Clockwise90 => (&tr_rect, &bl_rect, &tl_rect, &br_rect), + BasicRotationAngle::Clockwise180 => (&br_rect, &tl_rect, &tr_rect, &bl_rect), + BasicRotationAngle::Clockwise270 => (&bl_rect, &tr_rect, &br_rect, &tl_rect), + }; + + let dummy_mask_image = resource_cache.get_dummy_mask_image(); + + // Draw the rounded part of the corner. for rect_index in 0..tessellator::quad_count_for_border_corner(outer_radius) { - let tessellated_rect = vertices_rect.tessellate_border_corner(outer_radius, - inner_radius, - rotation_angle, - rect_index); + let tessellated_rect = border_corner_rect.tessellate_border_corner(outer_radius, + inner_radius, + rotation_angle, + rect_index); let mask_image = match BorderRadiusRasterOp::create(outer_radius, inner_radius, false, - rect_index, + Some(rect_index), ImageFormat::A8) { Some(raster_item) => { - let raster_item = RasterItem::BorderRadius(raster_item); - resource_cache.get_raster(&raster_item) - } - None => { - resource_cache.get_dummy_mask_image() + resource_cache.get_raster(&RasterItem::BorderRadius(raster_item)) } + None => dummy_mask_image, }; // FIXME(pcwalton): Either use RGBA8 textures instead of alpha masks here, or implement @@ -1050,71 +1100,118 @@ impl<'a> BatchBuilder<'a> { varyings: mask_uv, }; - clipper::clip_rect_to_combined_region(tessellated_rect, - &mut clip_buffers.sh_clip_buffers, - &mut clip_buffers.rect_pos_uv, - clip); - - for clip_region in clip_buffers.rect_pos_uv - .clip_rect_to_region_result_output - .drain(..) { - let rect_pos_uv = &clip_region.rect_result; - let v0; - let v1; - let muv0; - let muv1; - let muv2; - let muv3; - match rotation_angle { - BasicRotationAngle::Upright => { - v0 = rect_pos_uv.pos.origin; - v1 = rect_pos_uv.pos.bottom_right(); - muv0 = rect_pos_uv.varyings.top_left; - muv1 = rect_pos_uv.varyings.top_right; - muv2 = rect_pos_uv.varyings.bottom_right; - muv3 = rect_pos_uv.varyings.bottom_left; - } - BasicRotationAngle::Clockwise90 => { - v0 = rect_pos_uv.pos.top_right(); - v1 = rect_pos_uv.pos.bottom_left(); - muv0 = rect_pos_uv.varyings.top_right; - muv1 = rect_pos_uv.varyings.top_left; - muv2 = rect_pos_uv.varyings.bottom_left; - muv3 = rect_pos_uv.varyings.bottom_right; - } - BasicRotationAngle::Clockwise180 => { - v0 = rect_pos_uv.pos.bottom_right(); - v1 = rect_pos_uv.pos.origin; - muv0 = rect_pos_uv.varyings.bottom_right; - muv1 = rect_pos_uv.varyings.bottom_left; - muv2 = rect_pos_uv.varyings.top_left; - muv3 = rect_pos_uv.varyings.top_right; - } - BasicRotationAngle::Clockwise270 => { - v0 = rect_pos_uv.pos.bottom_left(); - v1 = rect_pos_uv.pos.top_right(); - muv0 = rect_pos_uv.varyings.bottom_left; - muv1 = rect_pos_uv.varyings.bottom_right; - muv2 = rect_pos_uv.varyings.top_right; - muv3 = rect_pos_uv.varyings.top_left; - } - } + self.add_border_corner_piece(tessellated_rect, + clip, + mask_image, + color0, + color1, + resource_cache, + clip_buffers, + rotation_angle); + } + + // Draw the inner rect. + self.add_border_corner_piece(RectPolygon { + pos: *inner_rect, + varyings: RectUv::zero(), + }, + clip, + dummy_mask_image, + color0, + color1, + resource_cache, + clip_buffers, + rotation_angle); + + // Draw the two solid rects. + if util::rect_is_well_formed_and_nonempty(color0_rect) { + self.add_color_rectangle(color0_rect, clip, resource_cache, clip_buffers, color0) + } + if util::rect_is_well_formed_and_nonempty(color1_rect) { + self.add_color_rectangle(color1_rect, clip, resource_cache, clip_buffers, color1) + } + } + + /// Draws one rectangle making up a border corner. + fn add_border_corner_piece(&mut self, + tessellated_rect: RectPolygon, + clip: &CombinedClipRegion, + mask_image: &TextureCacheItem, + color0: &ColorF, + color1: &ColorF, + resource_cache: &ResourceCache, + clip_buffers: &mut clipper::ClipBuffers, + rotation_angle: BasicRotationAngle) { + if !tessellated_rect.is_well_formed_and_nonempty() { + return + } + + let white_image = resource_cache.get_dummy_color_image(); + + clipper::clip_rect_to_combined_region(tessellated_rect, + &mut clip_buffers.sh_clip_buffers, + &mut clip_buffers.rect_pos_uv, + clip); - let mut vertices = [ - PackedVertex::from_components(v0.x, v0.y, color0, 0.0, 0.0, muv0.x, muv0.y), - PackedVertex::from_components(v1.x, v1.y, color0, 0.0, 0.0, muv2.x, muv2.y), - PackedVertex::from_components(v0.x, v1.y, color0, 0.0, 0.0, muv3.x, muv3.y), - PackedVertex::from_components(v0.x, v0.y, color1, 0.0, 0.0, muv0.x, muv0.y), - PackedVertex::from_components(v1.x, v0.y, color1, 0.0, 0.0, muv1.x, muv1.y), - PackedVertex::from_components(v1.x, v1.y, color1, 0.0, 0.0, muv2.x, muv2.y), - ]; - - self.add_draw_item(white_image.texture_id, - mask_image.texture_id, - Primitive::Triangles, - &mut vertices, - None); + for clip_region in clip_buffers.rect_pos_uv + .clip_rect_to_region_result_output + .drain(..) { + let rect_pos_uv = &clip_region.rect_result; + let v0; + let v1; + let muv0; + let muv1; + let muv2; + let muv3; + match rotation_angle { + BasicRotationAngle::Upright => { + v0 = rect_pos_uv.pos.origin; + v1 = rect_pos_uv.pos.bottom_right(); + muv0 = rect_pos_uv.varyings.top_left; + muv1 = rect_pos_uv.varyings.top_right; + muv2 = rect_pos_uv.varyings.bottom_right; + muv3 = rect_pos_uv.varyings.bottom_left; + } + BasicRotationAngle::Clockwise90 => { + v0 = rect_pos_uv.pos.top_right(); + v1 = rect_pos_uv.pos.bottom_left(); + muv0 = rect_pos_uv.varyings.top_right; + muv1 = rect_pos_uv.varyings.top_left; + muv2 = rect_pos_uv.varyings.bottom_left; + muv3 = rect_pos_uv.varyings.bottom_right; + } + BasicRotationAngle::Clockwise180 => { + v0 = rect_pos_uv.pos.bottom_right(); + v1 = rect_pos_uv.pos.origin; + muv0 = rect_pos_uv.varyings.bottom_right; + muv1 = rect_pos_uv.varyings.bottom_left; + muv2 = rect_pos_uv.varyings.top_left; + muv3 = rect_pos_uv.varyings.top_right; + } + BasicRotationAngle::Clockwise270 => { + v0 = rect_pos_uv.pos.bottom_left(); + v1 = rect_pos_uv.pos.top_right(); + muv0 = rect_pos_uv.varyings.bottom_left; + muv1 = rect_pos_uv.varyings.bottom_right; + muv2 = rect_pos_uv.varyings.top_right; + muv3 = rect_pos_uv.varyings.top_left; + } } + + let mut vertices = [ + PackedVertex::from_components(v0.x, v0.y, color0, 0.0, 0.0, muv0.x, muv0.y), + PackedVertex::from_components(v1.x, v1.y, color0, 0.0, 0.0, muv2.x, muv2.y), + PackedVertex::from_components(v0.x, v1.y, color0, 0.0, 0.0, muv3.x, muv3.y), + PackedVertex::from_components(v0.x, v0.y, color1, 0.0, 0.0, muv0.x, muv0.y), + PackedVertex::from_components(v1.x, v0.y, color1, 0.0, 0.0, muv1.x, muv1.y), + PackedVertex::from_components(v1.x, v1.y, color1, 0.0, 0.0, muv2.x, muv2.y), + ]; + + self.add_draw_item(white_image.texture_id, + mask_image.texture_id, + Primitive::Triangles, + &mut vertices, + None); } } @@ -1242,6 +1339,8 @@ impl<'a> BatchBuilder<'a> { &Rect::new(tl_outer, Size2D::new(tl_inner.x - tl_outer.x, tl_inner.y - tl_outer.y)), + &Point2D::new(tl_outer.x + radius.top_left.width, + tl_outer.y + radius.top_left.height), &left_color, &top_color, &radius.top_left, @@ -1254,6 +1353,8 @@ impl<'a> BatchBuilder<'a> { &Rect::new(Point2D::new(tr_inner.x, tr_outer.y), Size2D::new(tr_outer.x - tr_inner.x, tr_inner.y - tr_outer.y)), + &Point2D::new(tr_outer.x - radius.top_right.width, + tl_outer.y + radius.top_right.height), &right_color, &top_color, &radius.top_right, @@ -1266,6 +1367,8 @@ impl<'a> BatchBuilder<'a> { &Rect::new(br_inner, Size2D::new(br_outer.x - br_inner.x, br_outer.y - br_inner.y)), + &Point2D::new(br_outer.x - radius.bottom_right.width, + br_outer.y - radius.bottom_right.height), &right_color, &bottom_color, &radius.bottom_right, @@ -1278,6 +1381,8 @@ impl<'a> BatchBuilder<'a> { &Rect::new(Point2D::new(bl_outer.x, bl_inner.y), Size2D::new(bl_inner.x - bl_outer.x, bl_outer.y - bl_inner.y)), + &Point2D::new(bl_outer.x + radius.bottom_left.width, + bl_outer.y - radius.bottom_left.height), &left_color, &bottom_color, &radius.bottom_left, @@ -1432,7 +1537,7 @@ fn mask_for_border_radius<'a>(resource_cache: &'a ResourceCache, inner_radius_x: Au(0), inner_radius_y: Au(0), inverted: inverted, - index: 0, + index: None, image_format: ImageFormat::A8, })) } diff --git a/src/internal_types.rs b/src/internal_types.rs index c57faade2e..af2cabce18 100644 --- a/src/internal_types.rs +++ b/src/internal_types.rs @@ -247,7 +247,7 @@ pub enum TextureUpdateDetails { Blit(Vec), Blur(Vec, Size2D, Au, TextureImage, TextureImage), /// All four corners, the tessellation index, and whether inverted, respectively. - BorderRadius(Au, Au, Au, Au, u32, bool), + BorderRadius(Au, Au, Au, Au, Option, bool), /// Blur radius, border radius, box rect, raster origin, and whether inverted, respectively. BoxShadow(Au, Au, Rect, Point2D, bool), } @@ -614,6 +614,12 @@ pub struct RectPolygon { pub varyings: Varyings, } +impl RectPolygon { + pub fn is_well_formed_and_nonempty(&self) -> bool { + util::rect_is_well_formed_and_nonempty(&self.pos) + } +} + #[derive(Clone, Copy, Debug)] pub struct RectColorsUv { pub colors: RectColors, @@ -648,6 +654,15 @@ pub struct RectUv { } impl RectUv { + pub fn zero() -> RectUv { + RectUv { + top_left: Point2D::new(0.0, 0.0), + top_right: Point2D::new(0.0, 0.0), + bottom_left: Point2D::new(0.0, 0.0), + bottom_right: Point2D::new(0.0, 0.0), + } + } + pub fn from_image_and_rotation_angle(image: &TextureCacheItem, rotation_angle: BasicRotationAngle, flip_90_degree_rotations: bool) @@ -820,7 +835,7 @@ pub struct BorderRadiusRasterOp { pub outer_radius_y: Au, pub inner_radius_x: Au, pub inner_radius_y: Au, - pub index: u32, + pub index: Option, pub image_format: ImageFormat, pub inverted: bool, } @@ -829,7 +844,7 @@ impl BorderRadiusRasterOp { pub fn create(outer_radius: &Size2D, inner_radius: &Size2D, inverted: bool, - index: u32, + index: Option, image_format: ImageFormat) -> Option { if outer_radius.width > 0.0 || outer_radius.height > 0.0 { diff --git a/src/renderer.rs b/src/renderer.rs index bc0a61b967..031be72409 100644 --- a/src/renderer.rs +++ b/src/renderer.rs @@ -565,12 +565,16 @@ impl Renderer { let tessellated_rect = Rect::new(Point2D::new(0.0, 0.0), Size2D::new(outer_rx, outer_ry)); - let tessellated_rect = - tessellated_rect.tessellate_border_corner( - &Size2D::new(outer_rx, outer_ry), - &Size2D::new(inner_rx, inner_ry), - BasicRotationAngle::Upright, - index); + let tessellated_rect = match index { + None => tessellated_rect, + Some(index) => { + tessellated_rect.tessellate_border_corner( + &Size2D::new(outer_rx, outer_ry), + &Size2D::new(inner_rx, inner_ry), + BasicRotationAngle::Upright, + index) + } + }; let border_position = Point2D::new(x - tessellated_rect.origin.x + outer_rx, y - tessellated_rect.origin.y + outer_ry); diff --git a/src/resource_list.rs b/src/resource_list.rs index c2d7c84f96..709abcdcc9 100644 --- a/src/resource_list.rs +++ b/src/resource_list.rs @@ -55,7 +55,7 @@ impl ResourceList { outer_radius: &Size2D, inner_radius: &Size2D, inverted: bool, - index: u32, + index: Option, image_format: ImageFormat) { if let Some(raster_item) = BorderRadiusRasterOp::create(outer_radius, inner_radius, @@ -66,12 +66,13 @@ impl ResourceList { } } + /// NB: Only adds non-tessellated border radii. pub fn add_radius_raster_for_border_radii(&mut self, radii: &BorderRadius) { let zero_size = Size2D::new(0.0, 0.0); - self.add_radius_raster(&radii.top_left, &zero_size, false, 0, ImageFormat::A8); - self.add_radius_raster(&radii.top_right, &zero_size, false, 0, ImageFormat::A8); - self.add_radius_raster(&radii.bottom_left, &zero_size, false, 0, ImageFormat::A8); - self.add_radius_raster(&radii.bottom_right, &zero_size, false, 0, ImageFormat::A8); + self.add_radius_raster(&radii.top_left, &zero_size, false, None, ImageFormat::A8); + self.add_radius_raster(&radii.top_right, &zero_size, false, None, ImageFormat::A8); + self.add_radius_raster(&radii.bottom_left, &zero_size, false, None, ImageFormat::A8); + self.add_radius_raster(&radii.bottom_right, &zero_size, false, None, ImageFormat::A8); } pub fn add_box_shadow_corner(&mut self, @@ -194,7 +195,7 @@ impl BuildRequiredResources for AABBTreeNode { resource_list.add_radius_raster(&info.radius.top_left, &info.top_left_inner_radius(), false, - rect_index, + Some(rect_index), ImageFormat::A8); } for rect_index in 0..tessellator::quad_count_for_border_corner( @@ -202,7 +203,7 @@ impl BuildRequiredResources for AABBTreeNode { resource_list.add_radius_raster(&info.radius.top_right, &info.top_right_inner_radius(), false, - rect_index, + Some(rect_index), ImageFormat::A8); } for rect_index in 0..tessellator::quad_count_for_border_corner( @@ -210,7 +211,7 @@ impl BuildRequiredResources for AABBTreeNode { resource_list.add_radius_raster(&info.radius.bottom_left, &info.bottom_left_inner_radius(), false, - rect_index, + Some(rect_index), ImageFormat::A8); } for rect_index in 0..tessellator::quad_count_for_border_corner( @@ -218,7 +219,7 @@ impl BuildRequiredResources for AABBTreeNode { resource_list.add_radius_raster(&info.radius.bottom_right, &info.bottom_right_inner_radius(), false, - rect_index, + Some(rect_index), ImageFormat::A8); } @@ -227,7 +228,7 @@ impl BuildRequiredResources for AABBTreeNode { info.top.width / 2.0), &Size2D::new(0.0, 0.0), false, - 0, + None, ImageFormat::RGBA8); } if info.right.style == BorderStyle::Dotted { @@ -235,7 +236,7 @@ impl BuildRequiredResources for AABBTreeNode { info.right.width / 2.0), &Size2D::new(0.0, 0.0), false, - 0, + None, ImageFormat::RGBA8); } if info.bottom.style == BorderStyle::Dotted { @@ -243,7 +244,7 @@ impl BuildRequiredResources for AABBTreeNode { info.bottom.width / 2.0), &Size2D::new(0.0, 0.0), false, - 0, + None, ImageFormat::RGBA8); } if info.left.style == BorderStyle::Dotted { @@ -251,7 +252,7 @@ impl BuildRequiredResources for AABBTreeNode { info.left.width / 2.0), &Size2D::new(0.0, 0.0), false, - 0, + None, ImageFormat::RGBA8); } } diff --git a/src/texture_cache.rs b/src/texture_cache.rs index 7daf9038fe..507e1a073a 100644 --- a/src/texture_cache.rs +++ b/src/texture_cache.rs @@ -760,11 +760,18 @@ impl TextureCache { let rect = Rect::new(Point2D::new(0.0, 0.0), Size2D::new(op.outer_radius_x.to_f32_px(), op.outer_radius_y.to_f32_px())); - let tessellated_rect = rect.tessellate_border_corner( - &Size2D::new(op.outer_radius_x.to_f32_px(), op.outer_radius_y.to_f32_px()), - &Size2D::new(op.inner_radius_x.to_f32_px(), op.inner_radius_y.to_f32_px()), - BasicRotationAngle::Upright, - op.index); + let tessellated_rect = match op.index { + Some(index) => { + rect.tessellate_border_corner( + &Size2D::new(op.outer_radius_x.to_f32_px(), + op.outer_radius_y.to_f32_px()), + &Size2D::new(op.inner_radius_x.to_f32_px(), + op.inner_radius_y.to_f32_px()), + BasicRotationAngle::Upright, + index) + } + None => rect, + }; let width = tessellated_rect.size.width.round() as u32; let height = tessellated_rect.size.height.round() as u32; diff --git a/src/util.rs b/src/util.rs index c34bcc8e75..32c860106c 100644 --- a/src/util.rs +++ b/src/util.rs @@ -206,3 +206,10 @@ impl VaryingElement for (ColorF, Point2D) { pub fn rect_is_empty(rect: &Rect) -> bool { rect.size.width == Zero::zero() || rect.size.height == Zero::zero() } + +/// Returns true if the rectangle's width and height are both strictly positive and false +/// otherwise. +pub fn rect_is_well_formed_and_nonempty(rect: &Rect) -> bool { + rect.size.width > 0.0 && rect.size.height > 0.0 +} +