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.

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

Proposed fix to allow DynamicFont kerning to be re-enabled #38731

Closed
follower opened this issue May 14, 2020 · 4 comments
Closed

Proposed fix to allow DynamicFont kerning to be re-enabled #38731

follower opened this issue May 14, 2020 · 4 comments
Milestone

Comments

@follower
Copy link
Contributor


TL;DR: The Bug & Cause

  • DynamicFont kerning was removed by Fix font kerning #24777 due to issues with rendering some fonts as described in Weird kerning on labels #21965.

  • The underlying issue with the removed kerning-related code was that the call to FT_Get_Kerning() supplied arguments that were character codes rather than glyph indexes.

  • The observed impact of the issue was that the kerning value applied was intended for a different pair of characters than the pair displayed.

    For certain font & character combinations the difference between the correct & incorrect kerning values was significant. e.g. for PetitLatin.ttf the kerning for t| ("tee pipe") was used to kern ck which resulted in character overlap.

TL;DR: The Fix

Potential fix steps:

diff --git a/scene/resources/dynamic_font.cpp b/scene/resources/dynamic_font.cpp
index e165f7c34b..be6d6b517f 100644
--- a/scene/resources/dynamic_font.cpp
+++ b/scene/resources/dynamic_font.cpp
@@ -282,7 +282,7 @@ float DynamicFontAtSize::_get_kerning_advance(const DynamicFontAtSize *font, Cha
 
 	if (p_next) {
 		FT_Vector delta;
-		FT_Get_Kerning(font->face, p_char, p_next, FT_KERNING_DEFAULT, &delta);
+		FT_Get_Kerning(font->face, FT_Get_Char_Index(font->face, p_char), FT_Get_Char_Index(font->face, p_next), FT_KERNING_DEFAULT, &delta);
 		advance = (delta.x / 64.0) / oversampling;
 	}

This fix would both re-enable kerning[1] for DynamicFont and fix the previously observed kerning issue.

[1] For fonts with kern tables but still not for GPOS/kern tables.

TL;DR: Pretty pictures

Comparison screenshots based on HEAD 3.2 branch yesterday:

  • The bug (incorrect kerning applied):

    buggy-kern-tee-pipe-cropped

  • Current situation (no kerning applied):

    current--no-kern--tee-pipe--cropped

  • After fix applied (correct kerning applied):

    good--kern--tee-pipe--fixed--cropped

    Note the correct c+k kerning and kerning that reduces the gap for the u+c & t+| character pairs.


[WIP: Intend to add additional details & code references.]

@Anutrix
Copy link
Contributor

Anutrix commented May 16, 2020

Can you make a PR?

@follower
Copy link
Contributor Author

@Anutrix Yes, although I'm currently knee deep in yaks. :)

@akien-mga
Copy link
Member

Fixed by #49377.

@follower
Copy link
Contributor Author

Can confirm that as of Godot 3.4 beta 2 the official "ducky tee-pipe"(TM) kerning test now appears to pass. :)

ducky--tee-pipe--test--Screenshot from 2021-07-28 16 36 49

Font file from Petit Latin Font.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants