Fast

[forthy42]

All my tuning together

Ok, here's everything I so far wanted to do in one big lump of patchset.

1. errno-style error numbering
2. two-stage glyph and kerning lookup instead of linear search
3. avoid reloading fonts for every glyph

I've done strace on the demos/font example, and rendering takes 0.304s with the original "one open call per single glyph", and 0.0798s with "one open call per glyphs batch (one size)" as is now the default. The interface is backwards compatible, i.e. there is now a per-font mode when to open and close the file, and there are two new calls

texture_font_default_mode(font, [ MODE_AUTO_CLOSE, MODE_GLYPHS_CLOSE, MODE_FREE_CLOSE, MODE_MANUAL_CLOSE, MODE_ALWAYS_OPEN ])

and

texture_font_close(font, mode_for_font, mode_for_library)

The normal default for a font is MODE_AUTO_CLOSE, which will be raised to MODE_GLYPHS_CLOSE in texture_font_load_glyphs, and so mostly emulate the current behavior of open and freeing the font face for every glyph rendered. The default mode for the FT_Library is MODE_ALWAYS_OPEN, I don't see a point in closing and opening that structure. It's now a per-thread structure reused for every font created in the same thread, so if you want to render several texture atlases in parallel, make sure you create the font faces in the same thread you use it in to render glyphs.

What I still want to add is a mode for glyph texture coordinates, where the coordinates are stored as pixels, and the scaling is done in the shader program (optional feature). That allows to not worry about multiple fonts being in one atlas, and needing to resize all the glyphs when the atlas is full. Just update your shader parameter.

And a "needs synced with the texture" flag in the atlas, to avoid needless syncs. That's both quick to do, and I'll probably do it this evening.

The CMakeLists.txt now contains a configurable debug/release option, and also builds shared libraries. I have inserted some dummy version numbers (shared libraries need those), as I have no clue what version name I should start with ;-).

I've not merged in all changes into the harfbuzz stuff, and I really think the harfbuzz support should be an additional call, not a separate library with just one file being different, and even that file is mostly the same stuff, and causes maintenance problems when anything changes.

[adrianbroher]

in one big lump of patchset

Not interested at all.

[forthy42]

I can easily separate the individual improvements into separate patchsets, but my errno pull request is still pending, and I don't want to create non-mergeable patchsets... If you want to merge them one-by-one, merge the errno patchset first.

[aqilc]

This looks cool, will it never be merged?

[rougier]

Sorry, I just forgot about this PR... @forthy42 Still interested in having it merged?

[forthy42]

Yes, still interested.

[rougier]

Nice. I guess there are some changes needed to the merging of PR since yours. Could you have a look to check it this would require only cosmectic change or a complete overhaul of your PR?

[forthy42]

I'll have a look at the merge conflicts.

[forthy42]

So far, looks like mostly cosmetic changes.

[michaeltyson]

I just made my own pull request that avoids loading the font for every glyph before I saw this PR. I'd love to see this merged in, @rougier

[aqilc]

Yeah, me too! Faster font loading is always welcomed.

[rougier]

I think a rebase is needed.

[michaeltyson]

I think a rebase is needed.

Up for that, @forthy42 ? I could give it a crack too, but then I think it would complicate things because I’d have to create a separate PR. Hopefully third time’s the charm eh? 😄

[forthy42]

I think I have time for this today or tomorrow.

[forthy42]

Ok, one major source of conflict was double work: I improved the error handling, and you improved the error handling. The merge now has no conflicts, but fails the checks.

[michaeltyson]

Great, thanks for doing that! Have you got time to sort out the remaining stuff with the checks, or shall I dive in? (I think that'll mean forking your fork, and making my own composite PR once I'm done, though)

[forthy42]

Checks on Windows still fail. Problems: Doesn't find snprintf (should be in stdio.h), doesn't find (probably Linux-only).

[forthy42]

No conflicts, checks pass.

[forthy42]

Great, thanks for doing that! Have you got time to sort out the remaining stuff with the checks, or shall I dive in? (I think that'll mean forking your fork, and making my own composite PR once I'm done, though)

My PR includes emoji support; you can check if your PR does something better.

The load&close font for every glyph code is not well tested anymore, I suppose there are issues with that. If anyone wants to keep that mode, test it; otherwise the next PR should remove this mode.

[michaeltyson]

Cool, nicely done! I've just done a PR against your repo with some additional fixes. Trying out the emoji support next, if I can figure it out 😄

[michaeltyson]

Lovely! These changes brought a reasonably completed scene's initial setup down from 0.32s to .18s.

I had to make some additional changes to support a unified 4-element texture atlas correctly, but I might make a separate PR once this is merged to avoid complicating things too much.

[forthy42]

I tried Apple's icon fonts, and it works well when you check for the FT_HAS_FIXED_SIZES in both places where color fonts need special treatment. Works with Android fonts (the one I tested this with originally), too.

Thanks to Michael Tyson for pointing at the right places.

[michaeltyson]

I'm still getting "unused function" build warnings on the rol8 function in texture-font.c - that's why I'd combined rol8/ror8 in my PR, rather than having the enddianness check at the point of calling. If you don't like that idea, maybe even just remove rol8/ror8 (as they're only single-use one-line functions) and put the shift-and-or lines inline. I could override the build warning, but it's probably better to just restructure a little so we don't have to suppress warnings at all.

Otherwise, all looks good 👍

[forthy42]

I'm still getting "unused function" build warnings on the rol8 function in texture-font.c

I changed that to a single rol(in, x) function; that should fix that problem.

[michaeltyson]

After that last rol() build warning, I think that's it from my end – everything's working great here. When that last thing's fixed, and when you're satisfied, @rougier, I think it's ready to merge 🎉

Thanks for all your great work, @forthy42!

[michaeltyson]

Yay! Okay, that’s it for me - whenever you’re ready, @rougier

[forthy42]

The only thing I've still a problem is make test. First of all, the tga saved is upside down (which is to be expected from OpenGL), and needs a flip before comparing. So there needs to be some preprocessing, because convert -flip is much easier than affine transformation with compare. And then, a number of things have changed (upstream and in my code) that stops the images from comparing fine.

But I suggest to attack this part later.

[rougier]

Let's pull the trigger then! Many many thanks for your hard and nice work.

[Bromvlieg]

I've been using FetchContent_Declare with the repo url for over a year or so, so that I have latest version, but this merge seems to error the build :(

[rougier]

@Bromvlieg Thanks for the report @forthy42 Do you know if this is related to your code?

[forthy42]

No idea, sorry. This is a problem early on in the build process, and I don't even know why it expects a freetype-gld library instead of freetype-gl. Typo?

[Bromvlieg]

The d at the end is for debug builds. When i try it with release it finds the target, but throws an error:

D:\code\cpp\mainframe\out\build\x64-Release\_deps\freetype-gl-src\ftgl-utils.h(22): fatal error C1075: '{': no matching token found

Edit:

I forked the repo, and wanted to give a shot at fixing the header file, but it's too much of a chaos to understand what's going on. Seems like it's fixed if i put an extra } at the end in the #ifdef __cplusplus block. But seems like it's only required in the specific FTGL_ERRORDEF_ block?

Regarding the CMake target not existing there are several things in the CMakeLists.txt that frighten me, but I think this one is the issue, it'll never make a Debug build. I don't define freetype-gl_DEBUG, MSVC does that while invoking cmake with DEBUG or RELEASE so this should not be done ourselfs in the CMakeLists.txt.

if(freetype-gl_DEBUG)
    set(CMAKE_BUILD_TYPE Debug)
else()
    set(CMAKE_BUILD_TYPE Release)
endif()

If I have time tonight i might take a go at it.

[forthy42]

Yes, this looks like the extern "C" is done twice. Can you delete the second one? I think this all can go away:

#  ifdef __cplusplus
#   define FTGL_NEED_EXTERN_C
#ifndef IMPLEMENT_FREETYPE_GL
extern
#endif
"C" {
#  endif

[Bromvlieg]

I've removed the code and added it to the PR I've made.