[Impeller] Fix glyph sampling overlap

impeller/compiler/shader_lib/impeller/transform.glsl

@@ -13,22 +13,16 @@ vec2 IPVec2TransformPosition(mat4 matrix, vec2 point) {
 
 // Returns the transformed gl_Position for a given glyph position in a glyph
 // atlas.
-vec4 IPPositionForGlyphPosition(mat4 mvp, vec2 unit_vertex, vec2 glyph_position, vec2 glyph_size) {
-  vec4 translate = mvp[0] * glyph_position.x
-                 + mvp[1] * glyph_position.y
-                 + mvp[3];
-  mat4 translated_mvp = mat4(
-    mvp[0],
-    mvp[1],
-    mvp[2],
-    vec4(
-      translate.xyz,
-      mvp[3].w
-    )
-  );
-  return translated_mvp *
-         vec4(unit_vertex.x * glyph_size.x,
-              unit_vertex.y * glyph_size.y, 0.0, 1.0);
+vec4 IPPositionForGlyphPosition(mat4 mvp,
+                                vec2 unit_position,
+                                vec2 glyph_position,
+                                vec2 glyph_size) {
+  vec4 translate =
+      mvp[0] * glyph_position.x + mvp[1] * glyph_position.y + mvp[3];
+  mat4 translated_mvp =
+      mat4(mvp[0], mvp[1], mvp[2], vec4(translate.xyz, mvp[3].w));
+  return translated_mvp * vec4(unit_position.x * glyph_size.x,
+                               unit_position.y * glyph_size.y, 0.0, 1.0);
 }
 
 #endif

impeller/entity/contents/text_contents.cc

@@ -80,6 +80,7 @@ static bool CommonRender(
   SamplerDescriptor sampler_desc;
   sampler_desc.min_filter = MinMagFilter::kLinear;
   sampler_desc.mag_filter = MinMagFilter::kLinear;
+  sampler_desc.mip_filter = MipFilter::kNone;
 
   typename FS::FragInfo frag_info;
   frag_info.text_color = ToVector(color.Premultiply());
@@ -103,9 +104,9 @@ static bool CommonRender(
   // interpolated vertex information is also used in the fragment shader to
   // sample from the glyph atlas.
 
-  const std::vector<Point> unit_vertex_points = {
-      {0, 0}, {1, 0}, {0, 1}, {1, 1}};
-  const std::vector<uint32_t> indices = {0, 1, 2, 1, 2, 3};
+  const std::array<Point, 4> unit_points = {Point{0, 0}, Point{1, 0},
+                                            Point{0, 1}, Point{1, 1}};
+  const std::array<uint32_t, 6> indices = {0, 1, 2, 1, 2, 3};
 
   VertexBufferBuilder<typename VS::PerVertexData> vertex_builder;
 
@@ -127,7 +128,7 @@ static bool CommonRender(
 
   for (const auto& run : frame.GetRuns()) {
     auto font = run.GetFont();
-    auto glyph_size_ = ISize::Ceil(font.GetMetrics().GetBoundingBox().size);
+    auto glyph_size_ = font.GetMetrics().GetBoundingBox().size;
     auto glyph_size = Point{static_cast<Scalar>(glyph_size_.width),
                             static_cast<Scalar>(glyph_size_.height)};
     auto metrics_offset =
@@ -141,22 +142,20 @@ static bool CommonRender(
         return false;
       }
 
-      auto atlas_position =
-          atlas_glyph_pos->origin + Point{1 / atlas_glyph_pos->size.width,
-                                          1 / atlas_glyph_pos->size.height};
+      auto atlas_position = atlas_glyph_pos->origin;
       auto atlas_glyph_size =
           Point{atlas_glyph_pos->size.width, atlas_glyph_pos->size.height};
       auto offset_glyph_position = glyph_position.position + metrics_offset;
 
-      for (const auto& point : unit_vertex_points) {
+      for (const auto& point : unit_points) {
         typename VS::PerVertexData vtx;
-        vtx.unit_vertex = point;
-        vtx.glyph_position = offset_glyph_position;
-        vtx.glyph_size = glyph_size;
-        vtx.atlas_position = atlas_position;
-        vtx.atlas_glyph_size = atlas_glyph_size;
+        vtx.unit_position = point;
+        vtx.destination_position = offset_glyph_position + Point(0.5, 0.5);
+        vtx.destination_size = glyph_size - Point(1.0, 1.0);
+        vtx.source_position = atlas_position + Point(0.5, 0.5);
+        vtx.source_glyph_size = atlas_glyph_size - Point(1.0, 1.0);
         if constexpr (std::is_same_v<TPipeline, GlyphAtlasPipeline>) {
-          vtx.color_glyph =
+          vtx.has_color =
               glyph_position.glyph.type == Glyph::Type::kBitmap ? 1.0 : 0.0;
         }
         vertex_builder.AppendVertex(std::move(vtx));

impeller/entity/shaders/glyph_atlas.frag

@@ -7,27 +7,25 @@ uniform sampler2D glyph_atlas_sampler;
 uniform FragInfo {
   vec2 atlas_size;
   vec4 text_color;
-} frag_info;
+}
+frag_info;
 
-in vec2 v_unit_vertex;
-in vec2 v_atlas_position;
-in vec2 v_atlas_glyph_size;
-in float v_color_glyph;
+in vec2 v_unit_position;
+in vec2 v_source_position;
+in vec2 v_source_glyph_size;
+in float v_has_color;
 
 out vec4 frag_color;
 
 void main() {
-  vec2 scale_perspective = v_atlas_glyph_size / frag_info.atlas_size;
-  vec2 offset = v_atlas_position / frag_info.atlas_size;
-  if (v_color_glyph == 1.0) {
-    frag_color = texture(
-      glyph_atlas_sampler,
-      v_unit_vertex * scale_perspective + offset
-    );
+  vec2 uv_size = v_source_glyph_size / frag_info.atlas_size;
+  vec2 offset = v_source_position / frag_info.atlas_size;
+  if (v_has_color == 1.0) {
+    frag_color =
+        texture(glyph_atlas_sampler, v_unit_position * uv_size + offset);
   } else {
-    frag_color = texture(
-      glyph_atlas_sampler,
-      v_unit_vertex * scale_perspective + offset
-    ).aaaa * frag_info.text_color;
+    frag_color =
+        texture(glyph_atlas_sampler, v_unit_position * uv_size + offset).aaaa *
+        frag_info.text_color;
   }
 }

impeller/entity/shaders/glyph_atlas.vert

@@ -6,24 +6,26 @@
 
 uniform FrameInfo {
   mat4 mvp;
-} frame_info;
+}
+frame_info;
 
-in vec2 unit_vertex;
-in vec2 glyph_position;
-in vec2 glyph_size;
-in vec2 atlas_position;
-in vec2 atlas_glyph_size;
-in float color_glyph;
+in vec2 unit_position;
+in vec2 destination_position;
+in vec2 destination_size;
+in vec2 source_position;
+in vec2 source_glyph_size;
+in float has_color;
 
-out vec2 v_unit_vertex;
-out vec2 v_atlas_position;
-out vec2 v_atlas_glyph_size;
-out float v_color_glyph;
+out vec2 v_unit_position;
+out vec2 v_source_position;
+out vec2 v_source_glyph_size;
+out float v_has_color;
 
 void main() {
-  gl_Position = IPPositionForGlyphPosition(frame_info.mvp, unit_vertex, glyph_position, glyph_size);
-  v_unit_vertex = unit_vertex;
-  v_atlas_position = atlas_position;
-  v_atlas_glyph_size = atlas_glyph_size;
-  v_color_glyph = color_glyph;
+  gl_Position = IPPositionForGlyphPosition(
+      frame_info.mvp, unit_position, destination_position, destination_size);
+  v_unit_position = unit_position;
+  v_source_position = source_position;
+  v_source_glyph_size = source_glyph_size;
+  v_has_color = has_color;
 }

impeller/entity/shaders/glyph_atlas_sdf.frag

@@ -7,28 +7,35 @@ uniform sampler2D glyph_atlas_sampler;
 uniform FragInfo {
   vec2 atlas_size;
   vec4 text_color;
-} frag_info;
+}
+frag_info;
 
-in vec2 v_unit_vertex;
-in vec2 v_atlas_position;
-in vec2 v_atlas_glyph_size;
+in vec2 v_unit_position;
+in vec2 v_source_position;
+in vec2 v_source_glyph_size;
 
 out vec4 frag_color;
 
 void main() {
-  vec2 scale_perspective = v_atlas_glyph_size / frag_info.atlas_size;
-  vec2 offset = v_atlas_position / frag_info.atlas_size;
+  vec2 scale_perspective = v_source_glyph_size / frag_info.atlas_size;
+  vec2 offset = v_source_position / frag_info.atlas_size;
 
   // Inspired by Metal by Example's SDF text rendering shader:
   // https://github.com/metal-by-example/sample-code/blob/master/objc/12-TextRendering/TextRendering/Shaders.metal
 
   // Outline of glyph is the isocontour with value 50%
   float edge_distance = 0.5;
-  // Sample the signed-distance field to find distance from this fragment to the glyph outline
-  float sample_distance = texture(glyph_atlas_sampler, v_unit_vertex * scale_perspective + offset).a;
-  // Use local automatic gradients to find anti-aliased anisotropic edge width, cf. Gustavson 2012
+  // Sample the signed-distance field to find distance from this fragment to the
+  // glyph outline
+  float sample_distance =
+      texture(glyph_atlas_sampler, v_unit_position * scale_perspective + offset)
+          .a;
+  // Use local automatic gradients to find anti-aliased anisotropic edge width,
+  // cf. Gustavson 2012
   float edge_width = length(vec2(dFdx(sample_distance), dFdy(sample_distance)));
-  // Smooth the glyph edge by interpolating across the boundary in a band with the width determined above
-  float insideness = smoothstep(edge_distance - edge_width, edge_distance + edge_width, sample_distance);
+  // Smooth the glyph edge by interpolating across the boundary in a band with
+  // the width determined above
+  float insideness = smoothstep(edge_distance - edge_width,
+                                edge_distance + edge_width, sample_distance);
   frag_color = frag_info.text_color * insideness;
 }

impeller/entity/shaders/glyph_atlas_sdf.vert

@@ -6,21 +6,23 @@
 
 uniform FrameInfo {
   mat4 mvp;
-} frame_info;
+}
+frame_info;
 
-in vec2 unit_vertex;
-in vec2 glyph_position;
-in vec2 glyph_size;
-in vec2 atlas_position;
-in vec2 atlas_glyph_size;
+in vec2 unit_position;
+in vec2 destination_position;
+in vec2 destination_size;
+in vec2 source_position;
+in vec2 source_glyph_size;
 
-out vec2 v_unit_vertex;
-out vec2 v_atlas_position;
-out vec2 v_atlas_glyph_size;
+out vec2 v_unit_position;
+out vec2 v_source_position;
+out vec2 v_source_glyph_size;
 
 void main() {
-  gl_Position = IPPositionForGlyphPosition(frame_info.mvp, unit_vertex, glyph_position, glyph_size);
-  v_unit_vertex = unit_vertex;
-  v_atlas_position = atlas_position;
-  v_atlas_glyph_size = atlas_glyph_size;
+  gl_Position = IPPositionForGlyphPosition(
+      frame_info.mvp, unit_position, destination_position, destination_size);
+  v_unit_position = unit_position;
+  v_source_position = source_position;
+  v_source_glyph_size = source_glyph_size;
 }

impeller/fixtures/struct_def_bug.vert

@@ -10,25 +10,25 @@ uniform FrameInfo {
 
 in vec2 unit_vertex;
 in mat4 glyph_position; // <--- Causes multiple slots to be used and is a failure.
-in vec2 glyph_size;
-in vec2 atlas_position;
-in vec2 atlas_glyph_size;
+in vec2 destination_size;
+in vec2 source_position;
+in vec2 source_glyph_size;
 
 out vec2 v_unit_vertex;
-out vec2 v_atlas_position;
-out vec2 v_atlas_glyph_size;
+out vec2 v_source_position;
+out vec2 v_source_glyph_size;
 out vec2 v_atlas_size;
 out vec4 v_text_color;
 
 void main() {
   gl_Position = frame_info.mvp
               * glyph_position
-              * vec4(unit_vertex.x * glyph_size.x,
-                     unit_vertex.y * glyph_size.y, 0.0, 1.0);
+              * vec4(unit_vertex.x * destination_size.x,
+                     unit_vertex.y * destination_size.y, 0.0, 1.0);
 
   v_unit_vertex = unit_vertex;
-  v_atlas_position = atlas_position;
-  v_atlas_glyph_size = atlas_glyph_size;
+  v_source_position = source_position;
+  v_source_glyph_size = source_glyph_size;
   v_atlas_size = frame_info.atlas_size;
   v_text_color = frame_info.text_color;
 }

impeller/typographer/backends/skia/text_render_context_skia.cc

@@ -71,15 +71,19 @@ static size_t PairsFitInAtlasOfSize(const FontGlyphPair::Vector& pairs,
   glyph_positions.clear();
   glyph_positions.reserve(pairs.size());
 
+  // TODO(114563): We might be able to remove this per-glyph padding if we fix
+  //               the underlying causes of the overlap.
+  constexpr auto padding = 2;
+
   for (size_t i = 0; i < pairs.size(); i++) {
     const auto& pair = pairs[i];
     const auto glyph_size =
         ISize::Ceil(pair.font.GetMetrics().GetBoundingBox().size *
                     pair.font.GetMetrics().scale);
     SkIPoint16 location_in_atlas;
-    if (!rect_packer->addRect(glyph_size.width,   //
-                              glyph_size.height,  //
-                              &location_in_atlas  //
+    if (!rect_packer->addRect(glyph_size.width + padding,   //
+                              glyph_size.height + padding,  //
+                              &location_in_atlas            //
                               )) {
       return pairs.size() - i;
     }