Page MenuHome

Font preview for file editor
AbandonedPublic

Authored by Campbell Barton (campbellbarton) on Jan 16 2015, 7:34 PM.
Tokens
"Love" token, awarded by lightbwk."Love" token, awarded by leon_cheung."Like" token, awarded by dingto."Love" token, awarded by pablovazquez."Love" token, awarded by januz."Love" token, awarded by Severin.

Details

Summary

This patch is giving font preview capabilities to the file browser.

Especially when you search a font for motion graphics work and all other types of text usage in Blender this is very important and a much anticipated feature. Developed while being at the bcon14 and refined at home, I think it could go to master now.

Edit: I rewrote this functionality completely now. It's now handled via the thumb_manager and tied to Blenders thumbnail creation system. That means that all font files are generated via preview threads and (most important) cached. That means also that you can see all your font files now at once instead of only previewing the currently selected.

Hints:

  • The preview starts when you're in the "preview" mode of the file browser editor
  • If you see a font icon instead of a preview, then Freetype could not set the charmap appropriately (Unicode) and therefore not generate a preview.
  • The preview text is the fontname atm. This will change for sure in the future (I think about adding a user definable textbox in the file browser) but this will be a different commit.
  • Only horizontal 1 row texts are possible atm. Changing this will introduce more reworkings in the underlying font handling area but could be done too in the future.

Screenshot of the functionality in action:

Greetings, Thomas

Diff Detail

Repository
rB Blender
Branch
arcpatch-D1002

Event Timeline

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

Btw: the file_ops.c has not been changed - I'll omit whitespace changes before committing...

Julian Eisel (Severin) requested changes to this revision.EditedJan 20 2015, 3:03 PM
Julian Eisel (Severin) edited edge metadata.

The only functional concern I have right now is that I'd like to see the text drawn centered on thumbnail view. Other than that, just some really picky non-functional concerns.

source/blender/editors/space_file/file_draw.c
308

We usually place structs first and then ints/floats/etc

311

Prefer to use a pointer: uiFontStyle *fs = &style->widgetlabel;

314

Same here, structs first (okay this is pickyness-overload :P). You can also merge the MEM_callocN line from below with this.

323–326

BLI_rcti_init makes this a one-liner :)

328

Code style: "if (font->blf_id > -1) {"

618

Better place braces around the single conditions, easier to read

This revision now requires changes to proceed.Jan 20 2015, 3:03 PM

Will address all issues this evening, especially the "thumbnails" comment - didn't try that yet. In this view it would be possibly even better to increase the font drawing size too.

Is "structs first" in our coding guidelines? I was always trying to group parameters logical but it's no problem to rearrange them ;)

Thomas Beck (plasmasolutions) edited edge metadata.

Addressed all comments from Julian (including the thumbnail alignment) except one:

  • The FontStyle must not be a fs* - you would change the fontstyle in our internal font array permanently when you would do that (leading to blank boxes where the font name should be on subsequent drawing operations. So I left it as it was before..

Would be great when you could review it again before the weekend - I will commit it then - just before a new sneak peek :)

Julian Eisel (Severin) edited edge metadata.

(Mostly) LGTM

Re fs pointer, ah right, thought you would only read from fs, didn't saw it's accessed as well

source/blender/editors/space_file/file_draw.c
333

there should still be a space between the > and the -1 :P

346

same here

This revision is now accepted and ready to land.Jan 20 2015, 11:06 PM
Bastien Montagne (mont29) requested changes to this revision.Jan 21 2015, 1:24 PM
Bastien Montagne (mont29) edited edge metadata.

Arg… Should have had saw this earlier… This code is OK on the basic level (style, working code, etc.), but on design level it really does not do what I’d expect for a font preview:

  • Loading and releasing font in UI struct on all drawing is super hyper bad! You kept it under control by only showing preview for 'active' entry, but still…
  • Preview itself is not so good:
    • One shall not use random file name here, should always be the same text (ideally a full pangram [you know, 'quick brown fox…'], but doubt we'll have room for it).
    • All fonts shall be previewed, not only the active one.
    • Think we should not bother with preview, unless we are in 'preview' display, like for any other data type.

So what I would rather do here is, move font render code into the 'preview job' (the one also generating .blend and image/video previews), rendering a few selected chars into an image for each file. Might need quite a bit of work though, not sure we can use our font code this way currently. :/

This revision now requires changes to proceed.Jan 21 2015, 1:24 PM

So, just to give all followers a status on this topic. Mont and me discussed it in IRC now and I agree with Mont on both points (it's a hack and it should work for the complete fonts list). Therefore I bite the bullet and implement it - but this time the right way. News will follow next weekend (2015-02-01)..

Thomas Beck (plasmasolutions) edited edge metadata.

Complete rewrite of the functionality ... should now behave much better in terms of speed. I'm aiming at Blender 2.75 with this patch - our users need this feature.

Bastien Montagne (mont29) requested changes to this revision.Mar 29 2015, 4:40 PM
Bastien Montagne (mont29) edited edge metadata.

Overall, patch looks nice and works OK.

Aside from inlined remarks, would suggest to also render two lines of text (maybe with slightly smaller chars) - we have room for it. E.g. first line: Set of upper fonts (AIEWSG or something like that); lower line: set of lower fonts (akjmlé or so). Again, we do not have place for a full pangram, but using same letters (as much representative as possible) is important - non-latin chars/fonts will also be an issue, but not crucial for now I’d say.

source/blender/blenfont/intern/blf.c
851–855

Not sure about the benefit of adding that dummy wrapper here (even though other API calls do that, they do add some checkings around internal func…).

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

Please use C comments (/* ... */).

812

Careful with code style, should be if (!font) {

822–825

This should use UI Font theme color imho. White on a clear theme is likely to give issues… Try it with White Sky theme e.g.

source/blender/imbuf/intern/thumbs.c
348–351

I really do not like using font file name here. Fonts preview shall use the same set of chars, always (as already said in a previous comment). You need to be able to compare fonts between each other…

This revision now requires changes to proceed.Mar 29 2015, 4:40 PM

Campbell, would also like your opinion here. :)

Campbell Barton (campbellbarton) requested changes to this revision.Mar 30 2015, 1:52 AM
Campbell Barton (campbellbarton) edited edge metadata.

This patch really shouldn't have to modify BLF, or make BLF know about images.
We already have BLF writing into image buffers without having to make BLF depend on ImBuf.

Creating a preview is a high level function in the editors ED_fileselect_*, at least one of the editor functions?

Hi Cambell, will change the patch accordingly when I process all the other things. Thanks for the heads up - I agree with you about the BLF_* not having to know about the ImBuf.

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

For all devs that also saw https://developer.blender.org/rB5ff132182dbb089111cba3a80fa3a8eeb6009ec3 and thought we have to do it like so... Blender has different code styles in Cycles, BGE and the rest... is clear now after a short chat with Mont..

822–825

Agreed, will have to change a bit more though: As the font preview is cached on disk and the user can change the theme afterwards, the previews would then not be updated appropriately. To take care of that I have to take the color into account (md5) when creating the preview hashes... will do that!

Thomas Beck (plasmasolutions) edited edge metadata.

All review points addressed:

  • Moved from 1-liner to a 3-liner preview
  • Using TH_TEXT (file space theme context) now to make the font previews themable. This is done by using the OGL drawing code to render the previews (originally generated in white) in the theme color
  • Freed BLF from the knowledge of ImBuf struct
  • Made the wrapper useful by ensurin that an fbuf exists and the text is not null
  • Created a new method in ED_fileselect.h / .c that creates an ImBuf and returns it to the caller
  • Changed all C++ comments to C styled comments
  • Respect the code style

Q: What would you think which strings are appropriate for preview? Will change the existing ones if needed before I commit when I hear a "go"...

If anything is unclear or not proper just tell me, I'll change it asap.
Greetings, Thomas

Thomas Beck (plasmasolutions) edited edge metadata.
  • Tiny codestyle changes after a chat with Severin, thanks!
Campbell Barton (campbellbarton) requested changes to this revision.Apr 2 2015, 4:21 AM
Campbell Barton (campbellbarton) edited edge metadata.
Campbell Barton (campbellbarton) added inline comments.
source/blender/blenfont/intern/blf_font.c
799–832

As far as I can tell there is really no need for _any_ change to BLF regarding this patch. just use BLF_buffer and BLF_buffer_col.

This revision now requires changes to proceed.Apr 2 2015, 4:21 AM
source/blender/editors/space_file/filesel.c
769

Why is a float-buffer used here? (uses more memory and we're only showing the image in screen-space)

source/blender/imbuf/IMB_thumbs.h
58

Better just call THB_SOURCE_FONT ?

source/blender/imbuf/intern/thumbs.c
48

is this needed?

351

in that case better add BLI_assert(0); (also, C++ comments)

source/blender/blenfont/intern/blf_font.c
799–832

Wondering why this opportunity didn't come to my mind earlier! Will do that and revert BLF_* changes...

source/blender/editors/space_file/filesel.c
769

This had no specific cause - I didn't know that screen-space means char buffer.. .will change that.

source/blender/imbuf/intern/thumbs.c
48

Good catch! I previously read back theme colors here but changed it...will delete that.

351

Will do

Hi @Campbell Barton (campbellbarton),

I'm just processing all points and got a question concerning freeing the BLF_* from my preview generation method.

The problem
As I understood your comment, I should only use the high level BLF_ methods to create a font (get a fontid), draw the preview and free the font again. This introduces one major problem because we're creating the thumbnails in a threading manner. As there are many forks processing different font previews at the same time, the global_font[ ] is filled rapidly (currently only 16 max) even though I unload the font after the preview.

Possible fixes

  • Use the internal methods from blf_font inside my previous generation method (moved to filesel.c) but then I would have to include blf_internal.h and blf_internal_types.h (which I find is nonsense architecture wise). OR
  • Increase the global_font[ ] size to a much bigger size (in my folder are at least 300 fonts processed OR
  • We leave the method internal to blf_font and let all the magic happen there.

What's your opinion about that? Which route should I take to stay safe architecture wise...?

I didn't push my current diff yet because I don't want to push a version that does not work but if you want to see the problems just look there... http://www.pasteall.org/57632/diff

@Thomas Beck (plasmasolutions), okay, this is a bit more involved re: threading.

We could go about this, considered both these options.

Expose the API (partially)

Expose a part of the BLF api that loads fonts (parts of blf_internal.h, probably under a new API), Would want to keep the FontBLF struct opaque, but we could at least expose some of the functions which act on it.

Extend the API

Add a new function for font drawing which loads the font, draws and frees it. eg

BLF_draw_buffer_fontfile(..., const char *fontfile, int (draw_callback)(...), void *user_data)

This has the advantage that the changes to the API are isolated to a single function, and it can be reused in different places.

psudocode

void BLF_draw_buffer_fontfile(
        buffer info, const char *fontfile, ...,
        draw_callback, void *user_data)
{
    load font;

    BLFDrawData draw_data;

    while (draw_callback(&draw_data, user_data)) {
        draw font using draw data;  /* string, position, color... etc */
    }


    free font;
}

This way you would just have to define a callback which draws the preview and pass it to BLF_draw_buffer_fontfile,
This isn't really such a big task, the function would likely be <50 lines.

Thomas Beck (plasmasolutions) edited edge metadata.
  • All review points addressed
  • Tiny codestyle changes after a chat with Severin, thanks!
  • Font preview generation now back to BLF.

After a talk with Campbell we agreed on having it at BLF_font.c. It is needed here because we have a threaded preview process. Therefore, the internal global_fonts array would grow very fast temporarily.
Apart from that the generation is still done via a float buffer because in my tests especially fonts with a very narrow overall appearance looked terrible with char buffers.

Committed rB8c539b0ab521d442d88c70243c04cbb9e40fe412

Made some changes...

  • There is one necessary bad level call from (IMB -> BLF), (going from IMB -> ED is worse really), isolated code and marked the bad level call into a single file.
  • Replace byte buffer with float.
  • Image had artifacts at the edges from drawing onto black-alpha. (fill the image [1,1,1,0] to avoid).
  • Change preview text to be more generic.
  • Preview method now scales (so generating larger thumbnails is supported). - can request 512x512 image for eg, font size scales up to match.

Great work guys! I noticed it also supports preview for complex fonts like Chinese, Japanese and Korea, which is really cool. However, the preview text seems always be English. IMHO, for such fonts, it would make much more sense to use localized characters or symbols. I wonder if it would be possible, too?

Current display in BI:

Example in winOS:

Thomas Beck (plasmasolutions) edited edge metadata.EditedApr 8 2015, 3:25 PM

Hi @Leon Cheung (leon_cheung),

that's right - atm it's a hardcoded string. In the future I plan to support much more like sorting by font styles, a user definable preview text, and so on. But that's a topic after Bastians asset-experiments and therefore part of a completely new patch.

Greetings, Thomas

Ah I see. Thanks for the information. :D

These strings could go via translations too, it should only be a minor change.

Added translations here (rBf75bbe27e23d2f794).

Note currently only for french & japanese (as test cases), next update of PO's will add it for everybody (once translated of course).
Also, following Blender's approach to completely ignore i18n stuff when disabled, you have to enable translations to get it working (can still have everything in UI in English by disabling label & tooltips translations though).