Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use HarfBuzz's advances instead of FreeType's #639

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Use HarfBuzz's advances instead of FreeType's #639

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 24 additions & 67 deletions Samples/basic/harfbuzz/src/FontFaceHandleHarfBuzz.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,12 +68,10 @@ bool FontFaceHandleHarfBuzz::Initialize(FontFaceHandleFreetype face, int font_si
if (!FreeType::InitialiseFaceHandle(ft_face, font_size, glyphs, metrics, load_default_glyphs))
return false;

has_kerning = Rml::FreeType::HasKerning(ft_face);
FillKerningPairCache();

hb_font = hb_ft_font_create_referenced((FT_Face)ft_face);
RMLUI_ASSERT(hb_font != nullptr);
hb_font_set_ptem(hb_font, (float)font_size);
hb_ft_font_set_funcs(hb_font);

// Generate the default layer and layer configuration.
base_layer = GetOrCreateLayer(nullptr);
Expand All @@ -98,7 +96,7 @@ const FallbackFontGlyphMap& FontFaceHandleHarfBuzz::GetFallbackGlyphs() const
}

int FontFaceHandleHarfBuzz::GetStringWidth(StringView string, const TextShapingContext& text_shaping_context,
const LanguageDataMap& registered_languages, Character prior_character)
const LanguageDataMap& registered_languages, Character /*prior_character*/)
Copy link
Contributor Author

@TriangulumDesire TriangulumDesire Jul 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing to note is that this change will make the prior_character parameter unused.

Originally, this character was used to calculate any extra kerning that may have existed prior to this string being rendered (for example, in cases where a much larger, continuous string was split into several renders).

One possible way to reïmplement this would be to create a new string that is a combination of the prior character and the first character of the current string. We could shape this new string and then query the offsets of the second shaped glyph (if it exists) and use those offsets when positioning the first character. I'll leave it up to you if this is something you want to implement.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might cause some issues in the text inputs especially. When we select text there, we break a sentence up into pre-selection, selection, and post-selection parts. If the kerning between these parts are not the same when changing the selection, then the text will move around a bit.

This may also affect where we decide to place the text cursor from a mouse position. And also for breaking up text in normal text flow, there might be some differences.

When glancing over the harfbuzz API, I think I saw some of them taking whole strings for prior text. This is probably something that would be a lot more accurate if we used this with harfbuzz, since harfbuzz can combine several characters (code points) into single glyphs.

I think this change as it stands is a bit problematic for the above reasons. Maybe it is better to use Freetype kerning until we have solved this issue properly? Maybe you could test it with a text area and a font with kerning, to see how problematic it is?

Copy link
Contributor Author

@TriangulumDesire TriangulumDesire Jul 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent some time trying to resolve this but to no avail. I couldn't find any HarfBuzz functions that allow you to provide prior text or anything similar.

Firstly, I added the prior character to the shaping buffer before the rest of the text (and then ignored its advance when measuring). However, it became difficult to determine which shaped glyphs were part of the prior character and which were not. There was also the issue of ligatures. The font supports the ti ligature (where they become a single glyph). If I were to have a prior character of t and a first character of i, HarfBuzz would shape them into the ti ligature. Then, if I were to ignore measuring the first shaped glyph (which would normally correspond to the prior character), it would skip both the t and i entirely (since they are now a single glyph).

I also tried creating a second, smaller shaping buffer that only consisted of the prior character and first character of the string but ran into similar problems listed above.

I also reïnstated the FreeType kerning function and used that. It worked fine for the “LatoLatin” font (since it supports the kern table), but this method won't work for fonts that do not include a kern table (EB Garamond is one such font). It also has the same aforementioned ligature issues.

I did come across hb-font-get-glyph-kerning-for-direction, but I looked at its source and it only returns legacy kerning values (such as those from the kern table), making it functionally similar to FreeType's kerning functions.

I found this old issue that was posted by someone solving a similar problem. However, it seems that this is a much more complicated issue, and one that HarfBuzz doesn't seem to solve—some shaping results can require multiple characters of context.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the one I believe I was thinking of: hb_buffer_add_codepoints() (looks like this also applies to _add_utf8):

When shaping part of a larger text (e.g. a run of text from a paragraph), instead of passing just the substring corresponding to the run, it is preferable to pass the whole paragraph and specify the run start and length as item_offset and item_length , respectively, to give HarfBuzz the full context to be able, for example, to do cross-run Arabic shaping or properly handle combining marks at stat of run.

I'm not sure if this is actually useful to help solve this problem though. It does indeed sound like a complicated problem. I came to remember this blog post about text shaping and ligatures, which seems like a similar problem: https://faultlore.com/blah/text-hates-you/#style-can-change-mid-ligature

Once we get to text editing, there is also the problem of placing the text cursor, and moving them properly between glyphs and such: https://lord.io/text-editing-hates-you-too/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah. That function is for adding text to a buffer. When it comes to shaping, you process the entire buffer in one go. One method to try could be to do this and then get the shaped glyphs for the segment we care about, but the main problem is actually identifying the start and end of said segment. Regardless, it seems as if we'll need more than just the prior character to assist herewith.

In the meantime, what would you like me to do with this pull request? I could reïmplement the original FreeType kerning functions and use the value it returns. It won't be perfect, but it'll provide a decent enough estimate for now. I have tried selecting text in input elements, and whilst the text does jump around a bit if you displace kerning or cut a ligature, it is only a few pixels, and I wouldn't say it's that noticeable unless you're specifically looking for it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, on English the movement is relatively subtle, although not something I would use in production. I also tested on Hindi and Arabic, and they are very janky. I also tested this on master, and it's pretty much the same there. We don't really do anything to handle right-to-left, so selection on Arabic does weird things. And while Hindi at least works, it's moving about a lot. I guess the main reason is that we don't consider more than one character back. And also, we're allowed to move between code points that become merged glyphs, which allows you to corrupt these glyphs. So yeah, a lot of things to work on before this becomes very stable.

This PR definitely is the right direction. And considering it's not considerably worse, and it does look a lot better in normal rendering, I'm okay with merging this if you agree with that. I think people in the mean time will just have to be careful with input fields if they want to use this font engine. Basically, one would need to use a font without any kerning here.

Copy link
Contributor Author

@TriangulumDesire TriangulumDesire Jul 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right. I should have tested languages other than English in the input fields. I tried selecting Arabic and Hindi, and I can see what you mean by how janky they are. I also tried selecting these languages on the master branch, and these problems still occur—so this issue definitely transcends this pull request.

I'm fine with it being merged if you are, since most of these issues were happening before this PR. I'm now just thinking of ways to resolve this issue. The first option that comes to mind is to give the entire line to the font engine and then determine whole characters for selection using HarfBuzz cluster indices. However, determining position within a string using cluster indices is difficult, and this still has the issue of splitting ligatures (since even though they are two characters, they are a single cluster) and would require a lot of changes to RmlUi's internal text processing. We might end up requiring both the Unicode Text Segmentation algorithm and the Unicode Bidirectional Algorithm.

{
int width = 0;

Expand All @@ -109,10 +107,9 @@ int FontFaceHandleHarfBuzz::GetStringWidth(StringView string, const TextShapingC
hb_buffer_add_utf8(shaping_buffer, string.begin(), (int)string.size(), 0, (int)string.size());
hb_shape(hb_font, shaping_buffer, nullptr, 0);

FontGlyphIndex prior_glyph_codepoint = FreeType::GetGlyphIndexFromCharacter(ft_face, prior_character);

unsigned int glyph_count = 0;
hb_glyph_info_t* glyph_info = hb_buffer_get_glyph_infos(shaping_buffer, &glyph_count);
hb_glyph_position_t* glyph_positions = hb_buffer_get_glyph_positions(shaping_buffer, &glyph_count);

for (int g = 0; g < (int)glyph_count; ++g)
{
Expand All @@ -121,19 +118,19 @@ int FontFaceHandleHarfBuzz::GetStringWidth(StringView string, const TextShapingC
if (IsASCIIControlCharacter(character))
continue;

FontGlyphIndex glyph_codepoint = glyph_info[g].codepoint;
const FontGlyph* glyph = GetOrAppendGlyph(glyph_codepoint, character);
FontGlyphIndex glyph_index = glyph_info[g].codepoint;
const FontGlyph* glyph = GetOrAppendGlyph(glyph_index, character);
if (!glyph)
continue;

// Adjust the cursor for the kerning between this character and the previous one.
width += GetKerning(prior_glyph_codepoint, glyph_codepoint);

// Adjust the cursor for this character's advance.
width += glyph->advance;
width += (int)text_shaping_context.letter_spacing;
if (glyph_index != 0)
width += glyph_positions[g].x_advance >> 6;
else
// Use the unshaped advance for unsupported characters.
width += glyph->advance;

prior_glyph_codepoint = glyph_codepoint;
width += (int)text_shaping_context.letter_spacing;
}

hb_buffer_destroy(shaping_buffer);
Expand Down Expand Up @@ -265,7 +262,6 @@ int FontFaceHandleHarfBuzz::GenerateString(RenderManager& render_manager, Textur
RMLUI_ASSERT(geometry_index + num_textures <= (int)mesh_list.size());

line_width = 0;
FontGlyphIndex prior_glyph_codepoint = 0;

// Set the mesh and textures to the geometries.
for (int tex_index = 0; tex_index < num_textures; ++tex_index)
Expand All @@ -279,6 +275,7 @@ int FontFaceHandleHarfBuzz::GenerateString(RenderManager& render_manager, Textur

unsigned int glyph_count = 0;
hb_glyph_info_t* glyph_info = hb_buffer_get_glyph_infos(shaping_buffer, &glyph_count);
hb_glyph_position_t* glyph_positions = hb_buffer_get_glyph_positions(shaping_buffer, &glyph_count);

mesh_list[geometry_index].mesh.indices.reserve(string.size() * 6);
mesh_list[geometry_index].mesh.vertices.reserve(string.size() * 4);
Expand All @@ -291,25 +288,28 @@ int FontFaceHandleHarfBuzz::GenerateString(RenderManager& render_manager, Textur
if (IsASCIIControlCharacter(character))
continue;

FontGlyphIndex glyph_codepoint = glyph_info[g].codepoint;
const FontGlyph* glyph = GetOrAppendGlyph(glyph_codepoint, character);
FontGlyphIndex glyph_index = glyph_info[g].codepoint;
const FontGlyph* glyph = GetOrAppendGlyph(glyph_index, character);
if (!glyph)
continue;

// Adjust the cursor for the kerning between this character and the previous one.
line_width += GetKerning(prior_glyph_codepoint, glyph_codepoint);

ColourbPremultiplied glyph_color = layer_colour;
// Use white vertex colors on RGB glyphs.
if (layer == base_layer && glyph->color_format == ColorFormat::RGBA8)
glyph_color = ColourbPremultiplied(layer_colour.alpha, layer_colour.alpha);

layer->GenerateGeometry(&mesh_list[geometry_index], glyph_codepoint, character, Vector2f(position.x + line_width, position.y),
glyph_color);
Vector2f glyph_offset(glyph_positions[g].x_offset >> 6, glyph_positions[g].y_offset >> 6);
layer->GenerateGeometry(&mesh_list[geometry_index], glyph_index, character,
Vector2f(position.x + line_width, position.y) + glyph_offset, glyph_color);

// Adjust the cursor for this character's advance.
if (glyph_index != 0)
line_width += glyph_positions[g].x_advance >> 6;
else
// Use the unshaped advance for unsupported characters.
line_width += glyph->advance;

line_width += glyph->advance;
line_width += (int)text_shaping_context.letter_spacing;
prior_glyph_codepoint = glyph_codepoint;
}

geometry_index += num_textures;
Expand Down Expand Up @@ -383,49 +383,6 @@ bool FontFaceHandleHarfBuzz::AppendFallbackGlyph(Character character)
return false;
}

void FontFaceHandleHarfBuzz::FillKerningPairCache()
{
if (!has_kerning)
return;

static constexpr char32_t KerningCache_AsciiSubsetBegin = 32;
static constexpr char32_t KerningCache_AsciiSubsetLast = 126;

for (char32_t i = KerningCache_AsciiSubsetBegin; i <= KerningCache_AsciiSubsetLast; i++)
{
for (char32_t j = KerningCache_AsciiSubsetBegin; j <= KerningCache_AsciiSubsetLast; j++)
{
const bool first_iteration = (i == KerningCache_AsciiSubsetBegin && j == KerningCache_AsciiSubsetBegin);

// Fetch the kerning from the font face. Submit zero font size on subsequent iterations for performance reasons.
const int kerning = FreeType::GetKerning(ft_face, first_iteration ? metrics.size : 0,
FreeType::GetGlyphIndexFromCharacter(ft_face, Character(i)), FreeType::GetGlyphIndexFromCharacter(ft_face, Character(j)));
if (kerning != 0)
{
kerning_pair_cache.emplace(AsciiPair((i << 8) | j), KerningIntType(kerning));
}
}
}
}

int FontFaceHandleHarfBuzz::GetKerning(FontGlyphIndex lhs, FontGlyphIndex rhs) const
{
// Check if we have no kerning, or if we are have an unsupported character.
if (!has_kerning || lhs == 0 || rhs == 0)
return 0;

// See if the kerning pair has been cached.
const auto it = kerning_pair_cache.find(AsciiPair((int(lhs) << 8) | int(rhs)));
if (it != kerning_pair_cache.end())
{
return it->second;
}

// Fetch it from the font face instead.
const int result = FreeType::GetKerning(ft_face, metrics.size, lhs, rhs);
return result;
}

const FontGlyph* FontFaceHandleHarfBuzz::GetOrAppendGlyph(FontGlyphIndex glyph_index, Character& character, bool look_in_fallback_fonts)
{
if (glyph_index == 0 && look_in_fallback_fonts && character != Character::Replacement)
Expand Down
13 changes: 0 additions & 13 deletions Samples/basic/harfbuzz/src/FontFaceHandleHarfBuzz.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,12 +119,6 @@ class FontFaceHandleHarfBuzz : public Rml::NonCopyMoveable {
// Build and append fallback glyph to 'fallback_glyphs'.
bool AppendFallbackGlyph(Character character);

// Build a kerning cache for common characters.
void FillKerningPairCache();

// Return the kerning for a codepoint pair.
int GetKerning(FontGlyphIndex lhs, FontGlyphIndex rhs) const;

/// Retrieve a glyph from the given code index, building and appending a new glyph if not already built.
/// @param[in] glyph_index The glyph index.
/// @param[in-out] character The character codepoint, can be changed e.g. to the replacement character if no glyph is found..
Expand Down Expand Up @@ -168,13 +162,6 @@ class FontFaceHandleHarfBuzz : public Rml::NonCopyMoveable {
// Each font layer that generated geometry or textures, indexed by the font-effect's fingerprint key.
FontLayerCache layer_cache;

// Pre-cache kerning pairs for some ascii subset of all characters.
using AsciiPair = uint16_t;
using KerningIntType = int16_t;
using KerningPairs = UnorderedMap<AsciiPair, KerningIntType>;
KerningPairs kerning_pair_cache;

bool has_kerning = false;
bool is_layers_dirty = false;
int version = 0;

Expand Down
26 changes: 0 additions & 26 deletions Samples/basic/harfbuzz/src/FreeTypeInterface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,32 +80,6 @@ bool AppendGlyph(FontFaceHandleFreetype face, int font_size, FontGlyphIndex glyp
return true;
}

int GetKerning(FontFaceHandleFreetype face, int font_size, FontGlyphIndex lhs, FontGlyphIndex rhs)
{
FT_Face ft_face = (FT_Face)face;

RMLUI_ASSERT(FT_HAS_KERNING(ft_face));

// Set face size again in case it was used at another size in another font face handle.
// Font size value of zero assumes it is already set.
if (font_size > 0)
{
float bitmap_scaling_factor = 1.0f;
if (!SetFontSize(ft_face, font_size, bitmap_scaling_factor) || bitmap_scaling_factor != 1.0f)
return 0;
}

FT_Vector ft_kerning;

FT_Error ft_error = FT_Get_Kerning(ft_face, lhs, rhs,FT_KERNING_DEFAULT, &ft_kerning);

if (ft_error)
return 0;

int kerning = ft_kerning.x >> 6;
return kerning;
}

FontGlyphIndex GetGlyphIndexFromCharacter(FontFaceHandleFreetype face, Character character)
{
return FT_Get_Char_Index((FT_Face)face, (FT_ULong)character);
Expand Down
4 changes: 0 additions & 4 deletions Samples/basic/harfbuzz/src/FreeTypeInterface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,6 @@ bool InitialiseFaceHandle(FontFaceHandleFreetype face, int font_size, FontGlyphM
// Build a new glyph representing the given glyph index and append to 'glyphs'.
bool AppendGlyph(FontFaceHandleFreetype face, int font_size, FontGlyphIndex glyph_index, Character character, FontGlyphMap& glyphs);

// Returns the kerning between two characters given by glyph indices.
// 'font_size' value of zero assumes the font size is already set on the face, and skips this step for performance reasons.
int GetKerning(FontFaceHandleFreetype face, int font_size, FontGlyphIndex lhs, FontGlyphIndex rhs);

// Returns the corresponding glyph index from a character code.
FontGlyphIndex GetGlyphIndexFromCharacter(FontFaceHandleFreetype face, Character character);

Expand Down