Page MenuHome
Differential D15258

BLF: Fonts with FT_Face Optional
ClosedPublic
Actions

Authored by Harley Acheson (harley) on Jun 21 2022, 10:02 PM.

Details

Reviewers
Brecht Van Lommel (brecht)
Campbell Barton (campbellbarton)
Commits
rBc0845abd897f: BLF: Fonts with FT_Face Optional
Summary

Allow FontBLFs to exist with NULL FT_Face, added when actually needed.

With this patch installed, the fallback fonts do not load with FT_Face, and one is only created when the font is actually required. This not only speeds up startup, but unused language fonts will never be loaded.

Diff Detail

Repository
rB Blender

Event Timeline

Harley Acheson (harley) requested review of this revision.Jun 21 2022, 10:02 PM
Harley Acheson (harley) created this revision.
Harley Acheson (harley) mentioned this in D15235: BLF: New Font Stack for Better Language Coverage.Jun 21 2022, 10:11 PM
Harley Acheson (harley) edited the summary of this revision. (Show Details)Jun 22 2022, 3:24 AM
Harley Acheson (harley) updated this revision to Diff 52753.Jun 22 2022, 4:00 AM
Harley Acheson (harley) edited the summary of this revision. (Show Details)

Only drop the faces of the fallback members, not the main fonts.

Harley Acheson (harley) updated this revision to Diff 52911.Jun 26 2022, 7:41 PM
Harley Acheson (harley) retitled this revision from BLF Experiment: Fonts with FT_Face Optional to BLF WIP: Fonts with FT_Face Optional.

Now have coverage details for the suggested fonts in source, so we can have those without loading the faces. Startup is faster without loading the faces for all fonts. And unused fonts are never loaded.

Currently seems a bit ugly. I think I need to find a way to make it prettier and more obvious, otherwise might seems confusing and complex. Might just need better naming and good comments.

Harley Acheson (harley) updated this revision to Diff 52913.Jun 26 2022, 8:14 PM

Much simpler. If the font is in the static list of details, don't load the face, otherwise do.

Harley Acheson (harley) updated this revision to Diff 52914.Jun 26 2022, 8:57 PM

Ensuring that kerning cache table gets created. No need for flags in eFaceDetails, just use same last_resort detection.

Harley Acheson (harley) updated this revision to Diff 52915.Jun 26 2022, 9:04 PM
Harley Acheson (harley) retitled this revision from BLF WIP: Fonts with FT_Face Optional to BLF: Fonts with FT_Face Optional.

Improved comments.

Harley Acheson (harley) edited the summary of this revision. (Show Details)Jun 26 2022, 9:06 PM
Brecht Van Lommel (brecht) requested changes to this revision.Jun 27 2022, 4:26 PM

Approach looks fine, but some issues regarding error handling.

source/blender/blenfont/intern/blf_font.c
1162

This assumes blf_ensure_face can't fail, but it can leave font->face NULL currently.

1274

There should be some mechanism that prevents it from repeatedly trying to reload the same file on failure. Either with some boolean that indicates there was an error before or creating a dummy empty font face.

1286

This will end up calling FT_Done_Face a second time on the same font->face on blf_font_free, in some cases.

1378

Replace MEM_freeN with a call to blf_font_free. And then remove the call from FT_Done_Face from blf_ensure_face, leaving blf_font_free to clean up everything. That also prevent leaking font->filepath.

1442

Should this check if if(font->face), it's not clear this function accepts NULL.

This revision now requires changes to proceed.Jun 27 2022, 4:26 PM
Harley Acheson (harley) marked 5 inline comments as done.Jun 28 2022, 3:00 AM

completing current review items.

Harley Acheson (harley) updated this revision to Diff 52969.Jun 28 2022, 3:10 AM

@Brecht Van Lommel (brecht) - There should be some mechanism that prevents it from repeatedly trying to reload the same file on failure...

Doing this with (another) font flag keeps this entirely within blf_ensure_face. Seems to make sense.

Brecht Van Lommel (brecht) accepted this revision.Jun 28 2022, 3:22 PM
This revision is now accepted and ready to land.Jun 28 2022, 3:22 PM
Harley Acheson (harley) updated this revision to Diff 53302.Jul 6 2022, 8:05 PM

Updated to the current state of master.

Harley Acheson (harley) planned changes to this revision.Jul 7 2022, 11:34 PM

Harley: careful update to master required after Variable font commit

Harley Acheson (harley) updated this revision to Diff 53396.Jul 8 2022, 3:58 AM

Updated to the current state of master. Also moved a few more things into blf_ensure_face that can be avoided until a face is needed.

This revision is now accepted and ready to land.Jul 8 2022, 3:58 AM
Brecht Van Lommel (brecht) accepted this revision.Jul 8 2022, 2:42 PM
Harley Acheson (harley) mentioned this in T94030: Font Changes (Parent Task).Jul 11 2022, 6:12 PM
Closed by commit rBc0845abd897f: BLF: Fonts with FT_Face Optional (authored by Harley Acheson (harley)). · Explain WhyJul 29 2022, 2:52 AM
This revision was automatically updated to reflect the committed changes.
Harley Acheson (harley) added a commit: rBc0845abd897f: BLF: Fonts with FT_Face Optional.
Harley Acheson (harley) mentioned this in D13137: BLF: Implement FreeType Caching.Aug 2 2022, 6:58 PM

Revision Contents
Changeset List

PathSize
source/
blender/
blenfont/
intern/
184 lines
2 lines
2 lines
2 lines
6 lines

Diff 52915

View Options

source/blender/blenfont/intern/blf_font.c

Loading...
View Options

source/blender/blenfont/intern/blf_font_default.c

Loading...
View Options

source/blender/blenfont/intern/blf_glyph.c

Loading...
View Options

source/blender/blenfont/intern/blf_internal.h

Loading...
View Options

source/blender/blenfont/intern/blf_internal_types.h

Loading...