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

[3.2] Fix fallback emoji font color. #44212

Merged
merged 1 commit into from
Dec 9, 2020

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Dec 9, 2020

Use the font face with the found char not the base one to detect color.

@bruvzg bruvzg added this to the 3.2 milestone Dec 9, 2020
@@ -326,7 +326,7 @@ float DynamicFontAtSize::draw_char(RID p_canvas_item, const Point2 &p_pos, CharT
cpos.y -= font->get_ascent();
cpos.y += ch->v_align;
Color modulate = p_modulate;
if (FT_HAS_COLOR(face)) {
if (FT_HAS_COLOR(font->face)) {
Copy link
Member

Choose a reason for hiding this comment

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

Should the other calls to FT_HAS_COLOR(face) get the same treatment?

scene/resources/dynamic_font.cpp
169:    if (FT_HAS_COLOR(face) && face->num_fixed_sizes > 0) {
309:            int error = FT_Load_Char(face, p_char, FT_HAS_COLOR(face) ? FT_LOAD_COLOR : FT_LOAD_DEFAULT);
329:                    if (FT_HAS_COLOR(face)) {
598:    int error = FT_Load_Char(face, p_char, FT_HAS_COLOR(face) ? FT_LOAD_COLOR : FT_LOAD_DEFAULT | (font->force_autohinter ? FT_LOAD_FORCE_AUTOHINT : 0) | ft_hinting);

Copy link
Member Author

Choose a reason for hiding this comment

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

No, draw_char and get_char_size are the only methods that are always called for base data instance and then accessing fallbacks. The rest are called for the fallback data directly.

Copy link
Member

Choose a reason for hiding this comment

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

This one is in draw_char though, used for the outline:

309:            int error = FT_Load_Char(face, p_char, FT_HAS_COLOR(face) ? FT_LOAD_COLOR : FT_LOAD_DEFAULT);

Copy link
Member

Choose a reason for hiding this comment

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

This is with this PR when using emoji font as main font:
Screenshot_20201209_104018

Copy link
Member

Choose a reason for hiding this comment

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

This additional diff doesn't solve it though, might be more complex than that.

diff --git a/scene/resources/dynamic_font.cpp b/scene/resources/dynamic_font.cpp
index 7027660633..ac6798cc23 100644
--- a/scene/resources/dynamic_font.cpp
+++ b/scene/resources/dynamic_font.cpp
@@ -305,10 +305,10 @@ float DynamicFontAtSize::draw_char(RID p_canvas_item, const Point2 &p_pos, CharT
 
        // use normal character size if there's no outline character
        if (p_outline && !ch->found) {
-               FT_GlyphSlot slot = face->glyph;
-               int error = FT_Load_Char(face, p_char, FT_HAS_COLOR(face) ? FT_LOAD_COLOR : FT_LOAD_DEFAULT);
+               FT_GlyphSlot slot = font->face->glyph;
+               int error = FT_Load_Char(font->face, p_char, FT_HAS_COLOR(font->face) ? FT_LOAD_COLOR : FT_LOAD_DEFAULT);
                if (!error) {
-                       error = FT_Render_Glyph(face->glyph, FT_RENDER_MODE_NORMAL);
+                       error = FT_Render_Glyph(font->face->glyph, FT_RENDER_MODE_NORMAL);
                        if (!error) {
                                Character character = Character::not_found();
                                character = const_cast<DynamicFontAtSize *>(this)->_bitmap_to_character(slot->bitmap, slot->bitmap_top, slot->bitmap_left, slot->advance.x / 64.0);

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's how it should work, outlines do no work with bitmaps, and the aforementioned code is only for skipping bitmap character width when drawing an outline.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, thanks for clarifying.

Copy link
Member Author

Choose a reason for hiding this comment

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

For the reference there's no outline for bitmaps emoji in master either (and embedded SVG emoji is not yet supported by freetype).

Copy link
Member

Choose a reason for hiding this comment

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

And we have this issue to keep track of it: #34870

@akien-mga
Copy link
Member

For the reference (this was discussed on IRC but mentioning it here too), the issue was already fixed in the master branch as part of the CTL work / font refactoring.

@akien-mga akien-mga merged commit 84b62c0 into godotengine:3.2 Dec 9, 2020
@akien-mga
Copy link
Member

Thanks!

@bruvzg bruvzg deleted the color_font_fix_32 branch December 9, 2020 10:21
@Cheeseness
Copy link
Contributor

Many thanks!

@akien-mga akien-mga modified the milestones: 3.2, 3.3 Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants