[Impeller] Snap glyph positions to screen space pixels and map UVs correctly

impeller/aiks/canvas.cc

@@ -445,8 +445,7 @@ void Canvas::DrawTextFrame(const TextFrame& text_frame,
     auto cvg = text_contents->GetCoverage(test).value();
     color_text_contents->SetTextPosition(cvg.origin + position);
 
-    text_contents->SetInverseMatrix(
-        Matrix::MakeTranslation(Vector3(-cvg.origin.x, -cvg.origin.y, 0)));
+    text_contents->SetOffset(-cvg.origin);
     color_text_contents->SetTextContents(std::move(text_contents));
     color_text_contents->SetColorSourceContents(source());
 

impeller/entity/contents/text_contents.cc

@@ -62,8 +62,8 @@ void TextContents::SetInheritedOpacity(Scalar opacity) {
   inherited_opacity_ = opacity;
 }
 
-void TextContents::SetInverseMatrix(Matrix matrix) {
-  inverse_matrix_ = matrix;
+void TextContents::SetOffset(Vector2 offset) {
+  offset_ = offset;
 }
 
 std::optional<Rect> TextContents::GetCoverage(const Entity& entity) const {
@@ -74,20 +74,14 @@ std::optional<Rect> TextContents::GetCoverage(const Entity& entity) const {
   return bounds->TransformBounds(entity.GetTransformation());
 }
 
-static Vector4 PositionForGlyphPosition(const Matrix& translation,
-                                        Point unit_position,
-                                        Size destination_size) {
-  return translation * (unit_position * destination_size);
-}
-
 template <class TPipeline>
 static bool CommonRender(
     const ContentContext& renderer,
     const Entity& entity,
     RenderPass& pass,
     const Color& color,
     const TextFrame& frame,
-    const Matrix& inverse_matrix,
+    Vector2 offset,
     std::shared_ptr<GlyphAtlas>
         atlas,  // NOLINT(performance-unnecessary-value-param)
     Command& cmd) {
@@ -96,8 +90,8 @@ static bool CommonRender(
 
   // Common vertex uniforms for all glyphs.
   typename VS::FrameInfo frame_info;
-  frame_info.mvp = Matrix::MakeOrthographic(pass.GetRenderTargetSize()) *
-                   entity.GetTransformation();
+
+  frame_info.mvp = Matrix::MakeOrthographic(pass.GetRenderTargetSize());
   VS::BindFrameInfo(cmd, pass.GetTransientsBuffer().EmplaceUniform(frame_info));
 
   SamplerDescriptor sampler_desc;
@@ -148,45 +142,63 @@ static bool CommonRender(
   vertex_builder.Reserve(count * 4);
   vertex_builder.ReserveIndices(count * 6);
 
-  uint32_t offset = 0u;
+  uint32_t index_offset = 0u;
   for (auto i = 0u; i < count; i++) {
     for (const auto& index : indices) {
-      vertex_builder.AppendIndex(index + offset);
+      vertex_builder.AppendIndex(index + index_offset);
     }
-    offset += 4;
+    index_offset += 4;
   }
 
   auto atlas_size =
       Point{static_cast<Scalar>(atlas->GetTexture()->GetSize().width),
             static_cast<Scalar>(atlas->GetTexture()->GetSize().height)};
 
+  Vector2 screen_offset = (entity.GetTransformation() * offset).Round();
+
   for (const auto& run : frame.GetRuns()) {
     auto font = run.GetFont();
 
     for (const auto& glyph_position : run.GetGlyphPositions()) {
       FontGlyphPair font_glyph_pair{font, glyph_position.glyph};
-      auto atlas_glyph_pos = atlas->FindFontGlyphPosition(font_glyph_pair);
-      if (!atlas_glyph_pos.has_value()) {
+      auto atlas_glyph_bounds = atlas->FindFontGlyphBounds(font_glyph_pair);
+      if (!atlas_glyph_bounds.has_value()) {
         VALIDATION_LOG << "Could not find glyph position in the atlas.";
         return false;
       }
 
-      auto offset_glyph_position =
-          glyph_position.position + glyph_position.glyph.bounds.origin;
+      // For each glyph, we compute two rectangles. One for the vertex positions
+      // and one for the texture coordinates (UVs).
 
-      auto uv_scaler_a = atlas_glyph_pos->size / atlas_size;
-      auto uv_scaler_b = (Point::Round(atlas_glyph_pos->origin) / atlas_size);
-      auto translation =
-          Matrix::MakeTranslation(
-              Vector3(offset_glyph_position.x, offset_glyph_position.y, 0)) *
-          inverse_matrix;
+      auto uv_origin =
+          (atlas_glyph_bounds->origin - Point(0.5, 0.5)) / atlas_size;
+      auto uv_size = (atlas_glyph_bounds->size + Size(1, 1)) / atlas_size;
+
+      // Rounding here prevents most jitter between glyphs in the run when
+      // nearest sampling.
+      auto screen_glyph_position =
+          screen_offset +
+          (entity.GetTransformation().Basis() *
+           (glyph_position.position + glyph_position.glyph.bounds.origin))
+              .Round();
 
       for (const auto& point : unit_points) {
         typename VS::PerVertexData vtx;
-        auto position = PositionForGlyphPosition(
-            translation, point, glyph_position.glyph.bounds.size);
-        vtx.uv = point * uv_scaler_a + uv_scaler_b;
-        vtx.position = position;
+
+        if (entity.GetTransformation().IsTranslationScaleOnly()) {
+          // Rouding up here prevents the bounds from becoming 1 pixel too small
+          // when nearest sampling. This path breaks down for projections.
+          vtx.position =
+              screen_glyph_position + (entity.GetTransformation().Basis() *
+                                       point * glyph_position.glyph.bounds.size)
+                                          .Ceil();
+        } else {
+          vtx.position = entity.GetTransformation() *
+                         Vector4(offset + glyph_position.position +
+                                 glyph_position.glyph.bounds.origin +
+                                 point * glyph_position.glyph.bounds.size);
+        }
+        vtx.uv = uv_origin + point * uv_size;
 
         if constexpr (std::is_same_v<TPipeline, GlyphAtlasPipeline>) {
           vtx.has_color =
@@ -228,8 +240,8 @@ bool TextContents::RenderSdf(const ContentContext& renderer,
   cmd.pipeline = renderer.GetGlyphAtlasSdfPipeline(opts);
   cmd.stencil_reference = entity.GetStencilDepth();
 
-  return CommonRender<GlyphAtlasSdfPipeline>(
-      renderer, entity, pass, GetColor(), frame_, inverse_matrix_, atlas, cmd);
+  return CommonRender<GlyphAtlasSdfPipeline>(renderer, entity, pass, GetColor(),
+                                             frame_, offset_, atlas, cmd);
 }
 
 bool TextContents::Render(const ContentContext& renderer,
@@ -265,7 +277,7 @@ bool TextContents::Render(const ContentContext& renderer,
   cmd.stencil_reference = entity.GetStencilDepth();
 
   return CommonRender<GlyphAtlasPipeline>(renderer, entity, pass, color, frame_,
-                                          inverse_matrix_, atlas, cmd);
+                                          offset_, atlas, cmd);
 }
 
 }  // namespace impeller

impeller/entity/contents/text_contents.h

@@ -38,7 +38,7 @@ class TextContents final : public Contents {
 
   void SetInheritedOpacity(Scalar opacity) override;
 
-  void SetInverseMatrix(Matrix matrix);
+  void SetOffset(Vector2 offset);
 
   // |Contents|
   std::optional<Rect> GetCoverage(const Entity& entity) const override;
@@ -58,7 +58,7 @@ class TextContents final : public Contents {
   Color color_;
   Scalar inherited_opacity_ = 1.0;
   mutable std::shared_ptr<LazyGlyphAtlas> lazy_atlas_;
-  Matrix inverse_matrix_;
+  Vector2 offset_;
 
   std::shared_ptr<GlyphAtlas> ResolveAtlas(
       GlyphAtlas::Type type,

impeller/typographer/backends/skia/text_frame_skia.cc

@@ -76,7 +76,7 @@ TextFrame TextFrameFromTextBlob(const sk_sp<SkTextBlob>& blob, Scalar scale) {
                                  : Glyph::Type::kPath;
 
           text_run.AddGlyph(Glyph{glyphs[i], type, ToRect(glyph_bounds[i])},
-                            Point{point->x(), point->y()}.Round());
+                            Point{point->x(), point->y()}.Floor());
         }
         break;
       }

impeller/typographer/backends/skia/text_render_context_skia.cc

@@ -336,7 +336,7 @@ static bool UpdateAtlasBitmap(const GlyphAtlas& atlas,
   bool has_color = atlas.GetType() == GlyphAtlas::Type::kColorBitmap;
 
   for (const auto& pair : new_pairs) {
-    auto pos = atlas.FindFontGlyphPosition(pair);
+    auto pos = atlas.FindFontGlyphBounds(pair);
     if (!pos.has_value()) {
       continue;
     }

impeller/typographer/glyph_atlas.cc

@@ -70,7 +70,7 @@ void GlyphAtlas::AddTypefaceGlyphPosition(const FontGlyphPair& pair,
   positions_[pair] = rect;
 }
 
-std::optional<Rect> GlyphAtlas::FindFontGlyphPosition(
+std::optional<Rect> GlyphAtlas::FindFontGlyphBounds(
     const FontGlyphPair& pair) const {
   auto found = positions_.find(pair);
   if (found == positions_.end()) {

impeller/typographer/glyph_atlas.h

@@ -119,7 +119,7 @@ class GlyphAtlas {
   /// @return     The location of the font-glyph pair in the atlas.
   ///             `std::nullopt` of the pair in not in the atlas.
   ///
-  std::optional<Rect> FindFontGlyphPosition(const FontGlyphPair& pair) const;
+  std::optional<Rect> FindFontGlyphBounds(const FontGlyphPair& pair) const;
 
   //----------------------------------------------------------------------------
   /// @brief      whether this atlas contains all of the same font-glyph pairs