Page MenuHome

BLF: Refactor blf_glyph.c
ClosedPublic

Authored by Harley Acheson (harley) on Nov 4 2021, 12:21 AM.

Details

Summary

Cleanup and Simplification of blf_glyph.c


This cleanup should make this file a lot easier to understand. It breaks up some large functions into smaller modular pieces, that not only make it easier to follow but will also allow us to use them in interesting ways later.

blf_glyph_ensure now first calls blf_glyph_render, which returns a fully created glyph, and then blf_glyph_cache_add_glyph which adds the glyph to the cache.

blf_glyph_render itself is three parts. It calls blf_glyph_load to load the glyph data into the face, then it optionally transforms the outline, then it calls blf_glyph_render_bitmap to convert the curves into a bitmap that we can use.

Glyph indexes are found using blf_glyph_index_from_charcode, which is written to allow the font itself to change. We'll be able to easily demonstrate falling back from one to another when glyphs are not found.

The transform functions are written and accessed in a way to make it easier to support variable fonts later. One, blf_glyph_transform_monospace, when combined with fallback will allow glyphs from proportional fonts to be used harmoniously among monospaced fonts.

Having the functions broken up would allow us to load and render glyphs outside of our caching system, for a possible vfont replacement later.

So basically this should pretty it all up and leave it in a nicer shape for new features.

Diff Detail

Repository
rB Blender

Event Timeline

Harley Acheson (harley) requested review of this revision.Nov 4 2021, 12:21 AM
Harley Acheson (harley) created this revision.
Campbell Barton (campbellbarton) accepted this revision.EditedNov 4 2021, 4:18 AM

Minor request, otherwise LGTM.

  • GCC warns about casts, see: P2575
  • Two unused functions which could be left out of this patch or wrapped with UNUSED_FUNCTION, with explanation for their inclusion.
source/blender/blenfont/intern/blf_glyph.c
142–152

Shouldn't this return the glyph even when NULL?

This revision is now accepted and ready to land.Nov 4 2021, 4:18 AM
source/blender/blenfont/intern/blf_glyph.c
310

Unused function, wrap with UNUSED_FUNCTION() attribute.

321

Unused, wrap with UNUSED_FUNCTION() attribute.

329

The float/int casting in this function looks suspicious.

When GCC's warnings are resolved,

this line becomes: const FT_Pos extra_x = (int)((float)(gwidth - (int)width) * 5.65f);

Are all these conversions needed?

@Campbell Barton (campbellbarton) - Are all these conversions needed?

No, I should take out the transforms that are not used now. I mostly had to make sure that things that I could think of to do, or might want to do, would work there. And they plug in there fairly easily.

Shouldn't this return the glyph even when NULL?

I think that blf_glyph_search needs a better name and some comments. Its returning a glyph from the cache or NULL if its not in the cache. blf_glyph_cache_find_glyph or something. There seems to be a bit of muddiness with the blf_glyph_ and blf_glyph_cache names. I'll look at it again tomorrow with fresh eyes; staring at this too much today.

Updated to incorporate changes from review.

  • More and Improved comments.
  • Calm cast warnings.
  • Two unused functions marked with UNUSED_FUNCTION for now.

blf_glyph_search renamed to blf_glyph_cache_find_glyph to better reflect what it is doing.

@Campbell Barton (campbellbarton) - Shouldn't this return the glyph even when NULL?

Yes, sorry, didn't see this error. No need to look further if it should be in ascii table but is not.

Are all these conversions needed?

No, that was dumb. That passed width should have been int to match font->fixed_width.

Harley Acheson (harley) updated this revision to Diff 44447.EditedNov 6 2021, 1:10 AM

Small change to where thread locking occurs, to be more like how it is now. Didn't want to risk changing that.

Slight reordering in blf_glyph_render so that sizes of differing fonts are aligned before loading glyph.

This introduces a few magic numbers which can use some explanation, otherwise LGTM.

source/blender/blenfont/intern/blf_glyph.c
142

Redundant assignment, GlyphBLF *gc->bucket[blf_hash(charcode)].first; below instead.

378

Again, "About 6 degrees." is this some standard? or just rough value look looks OK?

382

Is 0.7 some typical value? or is this just eyeballing it?
Either way worth noting in comment.

@Campbell Barton (campbellbarton) - Redundant assignment

Awesome, thanks!

Again, "About 6 degrees." is this some standard? or just rough value look looks OK?
Is 0.7 some typical value? or is this just eyeballing it? Either way worth noting in comment.

Those comments are now Pulitzer prize-winning. Had to find my old notes and then double-check them.

They aren't eyeballed (I'm far too anal for that), but measured carefully against actual references. In this case between our current font and the real bold and oblique versions of it. But then checked against other font families to make sure the results are similar. Now well noted in the comments.

This revision was automatically updated to reflect the committed changes.