Details

Reviewers

Campbell Barton (campbellbarton)
Brecht Van Lommel (brecht)

Group Reviewers

User Interface

Commits

rB4e7983e07320: UI: Improved Font Thumbnails

Summary

Our current font previews are clever but not as useful as they could be.

We are currently showing a pangram ("Quick brown fox") over multiple lines in decreasing sizes. This text is translatable, so it can be different depending on the output language. But a primary reason to preview fonts is to get a sense of the style, proportions, stroke thicknesses, weight, obliqueness, etc. This can be seen much better and quicker with a smaller number of carefully-selected letters that are larger and aligned to a common baseline:

Current	Patch

We have been moving toward a more multi-language model. Users might be showing blender in a different language than their preferred language. And we allow any language input so while showing Spanish you can enter Chinese then Japanese text at any time. So tying the current text to the output language seems wrong, and is problematic that it necessarily clears all the thumbnails upon language change, requiring them to be reconstructed.

Further, it is much more important to show the contents of the font. If you download a specialty font (specific language, symbols, musical notes, etc) it is quite important to see that instantly. This patch will show letter samples from almost every language script on the planet. This becomes especially important for language fonts since they quite often contain latin letters as well - we really want to see the language features not the latin letters.

A good illustration of this difference is to compare how these look when the thumbnails are set to "Tiny":

Tested against thousands of different fonts. A comparison between current and patch while viewing Google Noto international fonts:

Current	Patch

There used to be a note here about performance, but the changes we have made in the last year have sped up everything enough that there is no longer a huge performance difference between this patch and current code. It is super-fast now either way.

Diff Detail

Repository: rB Blender

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

This seems like a good improvement, even if there are some minor trade-offs.

I don't think we should have a user preference for the sample text, especially as it's only for Latin fonts. Not worth adding more clutter to the preferences for that.

Other than it looks good to me.

source/blender/blenfont/intern/blf_thumbs.c
405	Functions in `blenfont` module should have `BLF` prefix.

This revision now requires changes to proceed.Aug 8 2022, 3:17 PM

Updated to current master.
Updated to use FreeType caching system
Removal of user preference for latin sample
New samples for Arabic and Devanagari
blf_get_sample_text moved to blf_thumbs.c since not shared.
IMB_thumb_load_font_get_hash moved back to current location in IMB thumbs_font.c

Harley Acheson (harley) edited the summary of this revision. (Show Details)Aug 9 2022, 1:22 AM

Harley Acheson (harley) marked 5 inline comments as done.Aug 9 2022, 1:24 AM

@Yevgeny Makarov (jenkm) - It crashes on Mac, unfortunately.

Could you give this patch another try on Mac?

Handle a font not returning a valid TT_OS2 font table.

In the case of a font not returning an OS2 table, make sure it still uses the default latin string and not just blank.

Not working correctly with the cache.

Harley: This looks like it can be done "bare metal", so don't use a FontBLF at all, open/close library, copy directly from FT_GlyphSlotRec->bitmap to supplied array. No locking needed. Should be as fast as could ever be and would not interfere if caching is added back in. Worth investigating anyway.

Now not using FontBLF at all, only direct FreeType calls.

This is faster as there is less work, no caching, etc.

No thread locking needed. Each preview gets its own FT_Library init and separate face. Then the bitmap created by FT_Render_Glyph is transferred directly to the preview bitmap.

No contention with the FreeType caching system if we want to add it again.

Fast, fast, fast.

Marking one function argument as UNUSED.

Slightly optimized cropping.

Harley Acheson (harley) edited the summary of this revision. (Show Details)Aug 12 2022, 4:20 AM

Handle the possibility of a non-scalable font having the initial size by fluke and then failing on final size.

Better samples for fonts with FT_ENCODING_MS_SYMBOL charmap.

Improved samples for Arabic, Urdu, and Carian.

Harley Acheson (harley) edited the summary of this revision. (Show Details)Aug 13 2022, 11:27 PM

Updated to the current state of master.

Pratik Borhade (PratikPB2123) added a subscriber: Pratik Borhade (PratikPB2123).Aug 27 2022, 5:00 AM

Seems to work fine on macOS.

Only some comments to make the code more robust to failures, especially for thumbnailing code we should be extra careful.

source/blender/blenfont/intern/blf_thumbs.c
238–240	The string should not be left uninitialized here.
295–299	Ensure the string is null terminated: memcpy(sample, string32, max_len - 1); sample[max_len - 1] = '\0'; However simpler would be to just return a `const char32_t*` from this function instead of making a copy.
323	If this one also fails, is it valid to call `FT_Set_Char_Size`?
349	The character map can be empty and still not return a glyph here, that failure case should be handled.
356	Handle failure case where font size ends up zero due to zero width or height.
380–388	When loading or rendering fails, return false instead of generating an empty thumbnail?

This revision now requires changes to proceed.Sep 7 2022, 3:12 PM

Updated to incorporate changes suggested by review.

Also added specific handling for "last resort" fonts, otherwise they test every range.

@Brecht Van Lommel (brecht) - Only some comments to make the code more robust to failures, especially for thumbnailing code we should be extra careful.

All great suggestions with only one thing not needed....

The character map can be empty and still not return a glyph here, that failure case should be handled.

There really is no need to worry about failure of FT_Get_Char_Index, FT_Get_Next_Char, or FT_Get_First_Char. They will always do something, at the very least return 0 for not found, but that properly (and always) maps to the "not.def" (tofu) character.

However simpler would be to just return a const char32_t* from this function instead of making a copy.

This did make it much simpler. Thanks!

When loading or rendering fails, return false instead of generating an empty thumbnail?

Great idea. I now keep track of the number of glyphs successfully loaded and rendered and return true if we got at least one.

Harley Acheson (harley) marked 6 inline comments as done.Sep 8 2022, 12:55 AM

Check for non-scalable fonts. Preview FT_ENCODING_MS_SYMBOL fonts that use f041- instead of 0041-

Just one more comment.

source/blender/blenfont/intern/blf_thumbs.c
328	Best not to hard code `10` here and make assumptions about the length of codepoints. Can you dynamically allocate this to the right size?

This revision now requires changes to proceed.Sep 8 2022, 6:58 PM

Updated to incorporate changes suggested by review.

Earlier exits for fonts that only have FT_ENCODING_MS_SYMBOL, non-Unicode charmaps, or no coverage bits.

@Brecht Van Lommel (brecht) - Best not to hard code 10 here and make assumptions about the length of codepoints. Can you dynamically allocate this to the right size?

Yes, that was dumb as those are all 32-bit codepoints and glyph indexes, nothing variable-length. I added a define for that length and also check against it while looping over them, so not just for null.

Harley Acheson (harley) marked an inline comment as done.Sep 8 2022, 8:49 PM

Just making "marlett.ttf", a very old MS font that is a wonky FT_ENCODING_MS_SYMBOL charset, look nicer.

Brecht Van Lommel (brecht) accepted this revision.Sep 9 2022, 2:44 PM

Brecht Van Lommel (brecht) added inline comments.

source/blender/blenfont/intern/blf_thumbs.c
408	No need to `#undef`.

Attempting to build, running into these warnings with GCC 12.2:

/src/blender/source/blender/blenfont/intern/blf_thumbs.c:224:33: error: \u0041 is not a valid universal character
  224 |     if (FT_Get_Char_Index(face, U'\u0041') != 0) {
      |                                 ^~~~~~~~~
/src/blender/source/blender/blenfont/intern/blf_thumbs.c:225:35: error: \u0041 is not a valid universal character
  225 |       return U"\u0041\u0044\u0048";
      |                                   ^
/src/blender/source/blender/blenfont/intern/blf_thumbs.c:225:35: error: \u0044 is not a valid universal character
/src/blender/source/blender/blenfont/intern/blf_thumbs.c:225:35: error: \u0048 is not a valid universal character

source/blender/blenfont/intern/blf_thumbs.c

37–41

e prefix is typically used for enums, no need for it here.

Unless this is unsigned, I'm geetting a warning.

/src/blender/source/blender/blenfont/intern/blf_thumbs.c:88:32: error: conversion from ‘long int’ to ‘int’ changes value from ‘2147483648’ to ‘-2147483648’ [-Werror=conversion]
   88 |     {U"\u1980\u1982\u1986", 3, TT_UCR_NEW_TAI_LUE},

Campbell Barton (campbellbarton) requested changes to this revision.Sep 9 2022, 3:30 PM

This revision now requires changes to proceed.Sep 9 2022, 3:30 PM

@Campbell Barton (campbellbarton) - \u0041 is not a valid universal character

Okay, that is pretty weird and interesting. From https://www.open-std.org/jtc1/sc22/wg14/www/docs/n717.htm, "A universal-character-name shall not specify a character short identifier in the ranges 0000 through 0020 or 007F through 009F, inclusive." But also seeing things elsewhere like "a UCN must not specify a value less than 00A0 other than 0024 ($), 0040 (@), or 0060 (?), nor a value in the range D800 through DFFF inclusive."

Obviously I could use a different format, but it turns out I didn't need anything in that range anyway. But interesting!

Harley Acheson (harley) marked 3 inline comments as done.Sep 9 2022, 6:50 PM

Updated to the current state of master.

@Campbell Barton (campbellbarton) - How are you with this change? Brecht is on board and it is quite popular. Might be nice to get into 3.4

Updated to the current state of master (renaming of filelist.c to filelist.cc)

This patch is required changes to build without warnings (P3210), there are some bit-shifts that would be good to avoid if possible when converting values from freetype to rounded integers (some can use existing ft_pix_* by the looks of it).

OTF/SyrCOMAdiabene.otf & OTF/SyrCOMAntioch.otf from https://archlinux.org/packages/extra/any/xorg-fonts-misc/ previously show preview text, but show blank images (some other fonts in this package too, those are just two from a handful that don't display, see:
. Perhaps entirely blank images could fall-back to English text.

This revision now requires changes to proceed.Sep 22 2022, 3:02 PM

@Campbell Barton (campbellbarton) - This patch is required changes to build without warnings

Thanks! I now have working BuildBot credentials so should be able to handle this better in future. I hope...

OTF/SyrCOMAdiabene.otf & OTF/SyrCOMAntioch.otf from https://archlinux.org/packages/extra/any/xorg-fonts-misc/ previously show preview text, but show blank images...

Crap, I'm not seeing that. I downloaded that package, un-zst, un-rared, and I see it like this:

Calm FT_Pos/ft_pix conversion warn/error.

Accepting, although I think this could use further improvements.

Previously when fonts failed to load they would show boxes (for unknown characters), now fonts show blank/nothing. Making it unclear if the font was corrupt or if the characters were missing.
How to handle situations where the characters are missing could be further investigated but I don't think needs to be part of this patch.
Some of the casting in this patch is quite awkward and can probably be simplified.
The issues I ran into with OTF/SyrCOMAdiabene.otf & OTF/SyrCOMAntioch.otf can be investigated in master - if other run into those problems and I don't see them as blocking.

This revision is now accepted and ready to land.Sep 23 2022, 3:56 AM

Last version was not outputting correctly to the output bitmap. I think I erased a section while cleaning up.

Calming Linux conversion warnings/errors.

Decrease likelihood of not finding glyphs.

Updated comments. Resetting FONT_THUMB_VERSION for release.

Closed by commit rB4e7983e07320: UI: Improved Font Thumbnails (authored by Harley Acheson (harley)). · Explain WhySep 24 2022, 7:57 PM

This revision was automatically updated to reflect the committed changes.

Harley Acheson (harley) added a commit: rB4e7983e07320: UI: Improved Font Thumbnails.

Improved Font Thumbnails
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 56082

source/blender/blenfont/BLF_api.h

source/blender/blenfont/intern/blf_font.c

source/blender/blenfont/intern/blf_internal.h

source/blender/blenfont/intern/blf_thumbs.c

source/blender/blentranslation/intern/blt_lang.c

source/blender/editors/space_file/filelist.cc

source/blender/imbuf/IMB_thumbs.h

source/blender/imbuf/intern/thumbs_font.c

source/blender/windowmanager/intern/wm_init_exit.c

Improved Font ThumbnailsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 56082

source/blender/blenfont/BLF_api.h

source/blender/blenfont/intern/blf_font.c

source/blender/blenfont/intern/blf_internal.h

source/blender/blenfont/intern/blf_thumbs.c

source/blender/blentranslation/intern/blt_lang.c

source/blender/editors/space_file/filelist.cc

source/blender/imbuf/IMB_thumbs.h

source/blender/imbuf/intern/thumbs_font.c

source/blender/windowmanager/intern/wm_init_exit.c

Improved Font Thumbnails
ClosedPublic
Actions

Revision Contents
Changeset List