diff --git a/impeller/entity/contents/text_contents.cc b/impeller/entity/contents/text_contents.cc index 1f30c9815582c..1aafcd4ae360e 100644 --- a/impeller/entity/contents/text_contents.cc +++ b/impeller/entity/contents/text_contents.cc @@ -128,10 +128,6 @@ static bool CommonRender( for (const auto& run : frame.GetRuns()) { auto font = run.GetFont(); - auto glyph_size_ = font.GetMetrics().GetBoundingBox().size; - auto glyph_size = Point{static_cast(glyph_size_.width), - static_cast(glyph_size_.height)}; - auto metrics_offset = font.GetMetrics().min_extent; for (const auto& glyph_position : run.GetGlyphPositions()) { FontGlyphPair font_glyph_pair{font, glyph_position.glyph}; @@ -144,13 +140,14 @@ static bool CommonRender( 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; + auto offset_glyph_position = + glyph_position.position + glyph_position.glyph.bounds.origin; for (const auto& point : unit_points) { typename VS::PerVertexData vtx; 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.destination_size = Point(glyph_position.glyph.bounds.size); 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) { diff --git a/impeller/typographer/backends/skia/text_frame_skia.cc b/impeller/typographer/backends/skia/text_frame_skia.cc index 7fc4f5fd977bd..bd082ffb71c76 100644 --- a/impeller/typographer/backends/skia/text_frame_skia.cc +++ b/impeller/typographer/backends/skia/text_frame_skia.cc @@ -27,26 +27,14 @@ static Font ToFont(const SkTextBlobRunIterator& run, Scalar scale) { Font::Metrics metrics; metrics.scale = scale; metrics.point_size = font.getSize(); - metrics.ascent = sk_metrics.fAscent; - metrics.descent = sk_metrics.fDescent; - metrics.min_extent = {sk_metrics.fXMin, sk_metrics.fTop}; - metrics.max_extent = {sk_metrics.fXMax, sk_metrics.fBottom}; - - std::vector glyph_bounds; - SkPaint paint; - - glyph_bounds.resize(run.glyphCount()); - run.font().getBounds(run.glyphs(), run.glyphCount(), glyph_bounds.data(), - nullptr); - for (auto& bounds : glyph_bounds) { - metrics.min_extent = metrics.min_extent.Min({bounds.fLeft, bounds.fTop}); - metrics.max_extent = - metrics.max_extent.Max({bounds.fRight, bounds.fBottom}); - } return Font{std::move(typeface), metrics}; } +static Rect ToRect(const SkRect& rect) { + return Rect::MakeLTRB(rect.fLeft, rect.fTop, rect.fRight, rect.fBottom); +} + TextFrame TextFrameFromTextBlob(const sk_sp& blob, Scalar scale) { if (!blob) { return {}; @@ -71,7 +59,11 @@ TextFrame TextFrameFromTextBlob(const sk_sp& blob, Scalar scale) { case SkTextBlobRunIterator::kHorizontal_Positioning: FML_DLOG(ERROR) << "Unimplemented."; break; - case SkTextBlobRunIterator::kFull_Positioning: + case SkTextBlobRunIterator::kFull_Positioning: { + std::vector glyph_bounds; + glyph_bounds.resize(glyph_count); + run.font().getBounds(glyphs, glyph_count, glyph_bounds.data(), nullptr); + for (auto i = 0u; i < glyph_count; i++) { // kFull_Positioning has two scalars per glyph. const SkPoint* glyph_points = run.points(); @@ -80,10 +72,11 @@ TextFrame TextFrameFromTextBlob(const sk_sp& blob, Scalar scale) { ? Glyph::Type::kBitmap : Glyph::Type::kPath; - text_run.AddGlyph(Glyph{glyphs[i], type}, + text_run.AddGlyph(Glyph{glyphs[i], type, ToRect(glyph_bounds[i])}, Point{point->x(), point->y()}); } break; + } case SkTextBlobRunIterator::kRSXform_Positioning: FML_DLOG(ERROR) << "Unimplemented."; break; diff --git a/impeller/typographer/backends/skia/text_render_context_skia.cc b/impeller/typographer/backends/skia/text_render_context_skia.cc index 58754115fa7c4..cf999db213333 100644 --- a/impeller/typographer/backends/skia/text_render_context_skia.cc +++ b/impeller/typographer/backends/skia/text_render_context_skia.cc @@ -71,16 +71,16 @@ 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. + // TODO(bdero): We might be able to remove this per-glyph padding if we fix + // the underlying causes of the overlap. + // https://github.com/flutter/flutter/issues/114563 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); + ISize::Ceil((pair.glyph.bounds * pair.font.GetMetrics().scale).size); SkIPoint16 location_in_atlas; if (!rect_packer->addRect(glyph_size.width + padding, // glyph_size.height + padding, // @@ -288,13 +288,14 @@ static std::shared_ptr CreateAtlasBitmap(const GlyphAtlas& atlas, glyph_paint.setColor(glyph_color); canvas->resetMatrix(); canvas->scale(metrics.scale, metrics.scale); - canvas->drawGlyphs(1u, // count - &glyph_id, // glyphs - &position, // positions - SkPoint::Make(-metrics.min_extent.x, - -metrics.min_extent.y), // origin - sk_font, // font - glyph_paint // paint + canvas->drawGlyphs( + 1u, // count + &glyph_id, // glyphs + &position, // positions + SkPoint::Make(-font_glyph.glyph.bounds.GetLeft(), + -font_glyph.glyph.bounds.GetTop()), // origin + sk_font, // font + glyph_paint // paint ); return true; }); diff --git a/impeller/typographer/font.h b/impeller/typographer/font.h index 2830900b7de9f..3e56981206f52 100644 --- a/impeller/typographer/font.h +++ b/impeller/typographer/font.h @@ -38,46 +38,9 @@ class Font : public Comparable { /// The point size of the font. /// Scalar point_size = 12.0f; - //-------------------------------------------------------------------------- - /// The font ascent relative to the baseline. This is usually negative as - /// moving upwards (ascending) in an upper-left-origin coordinate system - /// yields smaller numbers. - /// - Scalar ascent = 0.0f; - //-------------------------------------------------------------------------- - /// The font descent relative to the baseline. This is usually positive as - /// moving downwards (descending) in an upper-left-origin coordinate system - /// yields larger numbers. - /// - Scalar descent = 0.0f; - //-------------------------------------------------------------------------- - /// The minimum glyph extents relative to the origin. Typically negative in - /// an upper-left-origin coordinate system. - /// - Point min_extent; - //-------------------------------------------------------------------------- - /// The maximum glyph extents relative to the origin. Typically positive in - /// an upper-left-origin coordinate system. - /// - Point max_extent; - - //-------------------------------------------------------------------------- - /// @brief The union of the bounding boxes of all the glyphs. - /// - /// @return The bounding box. - /// - constexpr Rect GetBoundingBox() const { - return Rect::MakeLTRB(min_extent.x, // - min_extent.y, // - max_extent.x, // - max_extent.y // - ); - } constexpr bool operator==(const Metrics& o) const { - return scale == o.scale && point_size == o.point_size && - ascent == o.ascent && descent == o.descent && - min_extent == o.min_extent && max_extent == o.max_extent; + return scale == o.scale && point_size == o.point_size; } }; @@ -113,6 +76,6 @@ class Font : public Comparable { template <> struct std::hash { constexpr std::size_t operator()(const impeller::Font::Metrics& m) const { - return fml::HashCombine(m.scale, m.point_size, m.ascent, m.descent); + return fml::HashCombine(m.scale, m.point_size); } }; diff --git a/impeller/typographer/glyph.h b/impeller/typographer/glyph.h index d377afac46537..82ba5e4b299d8 100644 --- a/impeller/typographer/glyph.h +++ b/impeller/typographer/glyph.h @@ -8,6 +8,7 @@ #include #include "flutter/fml/macros.h" +#include "impeller/geometry/rect.h" namespace impeller { @@ -23,11 +24,18 @@ struct Glyph { uint16_t index = 0; //------------------------------------------------------------------------------ - /// @brief Whether the glyph is a path or a bitmap. + /// @brief Whether the glyph is a path or a bitmap. /// Type type = Type::kPath; - Glyph(uint16_t p_index, Type p_type) : index(p_index), type(p_type) {} + //------------------------------------------------------------------------------ + /// @brief Visibility coverage of the glyph in text run space (relative to + /// the baseline, no scaling applied). + /// + Rect bounds; + + Glyph(uint16_t p_index, Type p_type, Rect p_bounds) + : index(p_index), type(p_type), bounds(p_bounds) {} }; } // namespace impeller diff --git a/impeller/typographer/text_frame.cc b/impeller/typographer/text_frame.cc index 30e304e1cbbd8..ff05c28f41061 100644 --- a/impeller/typographer/text_frame.cc +++ b/impeller/typographer/text_frame.cc @@ -14,10 +14,10 @@ std::optional TextFrame::GetBounds() const { std::optional result; for (const auto& run : runs_) { - const auto glyph_bounds = run.GetFont().GetMetrics().GetBoundingBox(); for (const auto& glyph_position : run.GetGlyphPositions()) { - Rect glyph_rect = Rect(glyph_position.position + glyph_bounds.origin, - glyph_bounds.size); + Rect glyph_rect = + Rect(glyph_position.position + glyph_position.glyph.bounds.origin, + glyph_position.glyph.bounds.size); result = result.has_value() ? result->Union(glyph_rect) : glyph_rect; } } diff --git a/impeller/typographer/typographer_unittests.cc b/impeller/typographer/typographer_unittests.cc index 51d5dfe9b1587..978f5d700ee05 100644 --- a/impeller/typographer/typographer_unittests.cc +++ b/impeller/typographer/typographer_unittests.cc @@ -108,8 +108,6 @@ TEST_P(TypographerTest, GlyphAtlasWithOddUniqueGlyphSize) { ASSERT_NE(atlas, nullptr); ASSERT_NE(atlas->GetTexture(), nullptr); - // The 3 unique glyphs should not evenly fit into a square texture, so we - // should have a rectangular one. ASSERT_EQ(atlas->GetTexture()->GetSize().width, atlas->GetTexture()->GetSize().height); } @@ -166,7 +164,7 @@ TEST_P(TypographerTest, GlyphAtlasWithLotsOfdUniqueGlyphSize) { ASSERT_NE(atlas, nullptr); ASSERT_NE(atlas->GetTexture(), nullptr); - ASSERT_EQ(atlas->GetTexture()->GetSize().width, + ASSERT_EQ(atlas->GetTexture()->GetSize().width * 2, atlas->GetTexture()->GetSize().height); }