Page MenuHome

Complex Text Layout
Needs ReviewPublic

Authored by Campbell Barton (campbellbarton) on Feb 17 2016, 8:57 AM.

Details

Reviewers
Mazoon (Mazoon)
Summary
beforeafter

Current Blender does not support languages that need complex text layout as in:

  • languages that are written from right to left (e.g. Hebrew).
  • text where characters needs to be merged into one glyph (e.g. ligature feature in Latin characters).
  • languages that need both features (e.g. Arabic).

This patch adds complex text layout feature to blender using raqm HOST-Oman/libraqm. Raqm is a small library that encapsulates the logic for complex text layout. It currently provides bidirectional text support (using FriBiDi), shaping (using HarfBuzz), with proper script itemization. By the virtue of this, raqm can support most writing systems covered by Unicode.

As a native Arabic speaker, I can tell that it is essential for Blender users that will need it to type in Arabic to have this feature, since, as you can see in the image above, Arabic text without this feature is almost unrecognisable. And, I know that is also true for many other languages.

So, if you could please review my patch and if there are any modifications/suggestions, I am willing to work on it until it is hopefully accepted. That will make Blender much more international and it'll make life much easier for international designers to use an open source 3D design software like Blender :)

Core modifications were in font.c and freetypefont.c, a new function to deal with
raqm is added in freetypefont.c
BLI_shape_text() where, input text and the FT_Face are given to raqm, which will do the layout and returns glyph information (e.g. x_offset, x_advance, index, cluster...). And, changing the main function in font.c, BKE_vfont_to_curve_ex() to deal mainly with glyphs instead of characters.

Regards,
Mazoon

Diff Detail

Repository
rB Blender
Branch
T

Event Timeline

Mazoon (Mazoon) retitled this revision from to Complex Text Layout .Feb 17 2016, 8:57 AM
Mazoon (Mazoon) updated this object.
Mazoon (Mazoon) updated this revision to Diff 6113.
Mazoon (Mazoon) updated this object.
Mazoon (Mazoon) updated this object.

Hi, this would be great to support.

However going over this patch I ran into various issues attempting to build and use it.
(noted in review).

CMakeLists.txt
882

I needed to add find_package(PkgConfig) before this for it to work.

Note that PkgConfig isn't well supported everywhere.
So likely we would write our own Find module.
See: https://developer.blender.org/diffusion/B/browse/master/build_files/cmake/Modules/FindSpacenav.cmake for reference.

882

Would prefer to make this an option eg: WITH_FONT_RAQM, so we can still compile without.
It can help for debugging as well as building on systems with a minimal feature set, where some libraries aren't available.

source/blender/blenkernel/intern/font.c
162–272

Could you explain why these changes are needed?

634

This causes problem with mem = ef->textbuf; since its assigning wchar_t * to unsigned int *.

Setting back to wchar_t fixes.

If we want to use unsigned int for unicode, we can - but would mean changes in many more places.

source/blender/blenlib/intern/freetypefont.c
100

This line is now giving me an assert.

BLI_assert failed: /src/blender/source/blender/blenlib/intern/BLI_ghash.c:433, ghash_insert_ex(), at '(gh->flag & GHASH_FLAG_ALLOW_DUPES) || (BLI_ghash_haskey(gh, key) == 0)'

You should be getting the error message in a debug build. Enable WITH_ASSERT_ABORT can help debugging too. (so you don't accidentally miss it).

504

Did you investigate doing this in BKE_vfont_to_curve_ex ?
Can imagine its not necessarily simple to do this, but could be worth doing this inline if its not causing problems.

API note, should name BLI_vfont_shape_text, with some explanation of what it does.

515–551

Having to load the font into memory on every draw is quite a big change (and potentially a performance hit).
Ideally this information could be loaded once and cached - as we do for the glyphs themselves.

Otherwise we might want to make supporting Complex Text Layout an option that can be toggled.

This revision now requires changes to proceed.Feb 17 2016, 10:10 AM
source/blender/blenkernel/intern/font.c
162–272

Because even if vfont is_builtin, it will still need to go through raqm and so the vfont->temp_pf is needed to be set.

It is used here:
freetypefont.c:537:

/* Load the font from memory */
            if (vfonts[i]->temp_pf) {
                err = FT_New_Memory_Face(library,
                                         vfonts[i]->temp_pf->data,
                                         vfonts[i]->temp_pf->size,
                                         0,
                                         &face);
                if (err) {
                    printf("Failed to load the Freetype font");
                    return NULL;
                }
source/blender/blenlib/intern/freetypefont.c
504

It is possible to do it in BKE_vfont_to_curve_ex() actually. The reason of doing it in freetypefont.c was, I thought that any function that will deal with free type library should be in freetypefont.c, and for raqm, it is needed to set FT_Faces hence, I did it in freetypefont.c.

Should I move it to BKE_vfont_to_curve_ex() ?

source/blender/blenkernel/intern/font.c
634

Then, maybe it is better to change it to uint32_t everywhere?
Because wchar_t on windows is 16 bits and it won't fit all unicode characters. So, we need to use something that will always fit 32 bits.

source/blender/blenkernel/intern/font.c
634

In that case we better make this change as a separate patch (since it will need changes to quite a few different parts of the API).

source/blender/blenlib/intern/freetypefont.c
504

If it can be moved inside BKE_vfont_to_curve_ex without over-complicating the logic there, and avoiding to allocate an array, then I think this would be preferable.

source/blender/blenlib/intern/freetypefont.c
100

Hi @Campbell Barton (campbellbarton)
I couldn't fix this error. Could you please guide me on what might be the exact problem?

What I got is that before the insertion, it first checks if the value of the key is empty, and the error happens because it's not empty. But, BLI_ghash_insert() is only called by freetypechar_to_vchar() which is called after checking that the value of the key is NOT empty.
here freetypechar_to_vchar() is called:

if ((che = find_vfont_char(vfd, glyphs[i].index)) == NULL) {
	 che = BLI_vfontchar_from_freetypefont(vfont, glyphs[i].index);
}

So, it won't call it unless the value of the key is already empty! so how does the error (that it's not empty) occur later in BLI_ghash_insert() !

The only other place that can call freetypechar_to_vchar() is objfnt_to_ftvfontdata() when loading characters from a new font, but it uses a new ghash anyway so it should be empty since no insertion happened before.

/* Load characters */
	vfd->characters = BLI_ghash_int_new_ex(__func__, charcode_reserve);

	while (charcode < charcode_reserve) {
		/* Generate the font data */
		freetypechar_to_vchar(face, glyph_index, vfd);

I'm lost.

Mazoon (Mazoon) updated this revision to Diff 6214.
  • Added Findraqm.cmake.
  • Moved BLI_vfont_shape_text() code to BKE_vfont_to_curve_ex().
  • Setback w_char.

Commandeering, will make this library optional.

source/blender/blenlib/intern/freetypefont.c
100

This error is caused by the same key being added into the hash multiple times, am also not sure why its happening though - it needs some investigation to resolve.

Worth checking if its a race-condition (just a guess).

Campbell Barton (campbellbarton) retitled this revision from Complex Text Layout to Complex Text Layout.EditedApr 27 2016, 10:25 AM
  • tweak cmake raqm find module to work on my system (remove pkg-config use)
  • quiet some warnings.
  • make RAQM optional, though the ifdef isn't being checked at the moment.
  • sync with latest master.
  • rename vars to avoid diff noise.

Only add glyphs which arent already added

Characters may share a glyph

Running this patch with address sanitizer gives me an error: P354 (which looks to be an error in the use of the library, where we're attempting to read one past the last item in the array). (now fixed)

Testing without address sanitizer and I managed to paste in unicode Arabic text, and it seemed to work, though I cant judge this well.

@Mazoon (Mazoon). please commandeer back, so you can make updates to the patch.

  • test
  • Only add glyphs which arent already added
  • Merge branch 'master' into T

Merge branch 'master', fix off by one error

source/blender/blenkernel/intern/font.c
774

This is currently crashing when loading, text-regression.blend.

1147

shouldn't this be info = &custrinfo[glyphs[i].cluster]; ?

If not, would be good to explain why this is an exceptional case.

I was trying to get this patch to a level where it could be reviewed on a user level, but theres still crashes on more complex files.
Even so, on basic tests seem to work fine.

Even assuming this can be made to work acceptably, there is some other considerations.

Freetype usage

Currently fonts need to be kept in memory so and loaded/freed every update (each keystroke for example).
If keeping freetype font loaded can't be avoided, I think it would be better to keep it open (store a pointer from the VFont instead of re-loading the font every time).


Library Dependencies

Based on ArchLinux repo's, ( https://www.archlinux.org/packages/extra/i686/harfbuzz & https://www.archlinux.org/packages/extra/i686/fribidi/ )

This would make Blender depend on...

  • fribidi
    • glib2 (glib from GTK project, not glibc).
  • harfbuzz
    • graphite

As well as build-time dependencies (gobject-introspection, icu, ragl... ).

For Linux only, this isn't such a problem, but means pulling in low-level gtk/glib libraries for Windows and OSX too.

Before spending too much more time to finish off this patch, it would be good to decide if including these extra dependencies is acceptable.

Library Dependencies

Based on ArchLinux repo's, ( https://www.archlinux.org/packages/extra/i686/harfbuzz & https://www.archlinux.org/packages/extra/i686/fribidi/ )
This would make Blender depend on...

  • fribidi
    • glib2 (glib from GTK project, not glibc).
  • harfbuzz
    • graphite

As well as build-time dependencies (gobject-introspection, icu, ragl... ).
For Linux only, this isn't such a problem, but means pulling in low-level gtk/glib libraries for Windows and OSX too.
Before spending too much more time to finish off this patch, it would be good to decide if including these extra dependencies is acceptable.

glib2 & graphite are optional for fribidi [1] & harfbuzz [2]. Please see:
1- https://github.com/behdad/fribidi/blob/master/README#L24
2- https://github.com/behdad/harfbuzz/blob/fef5dd9a72f326c160a7194f558749d24bac7283/win32/README.txt#L37

Talked to @Campbell Barton (campbellbarton) in IRC. So basically this is what to be done to have functionality good for master:

  1. libraqm, fribidi and harfbuzz are to be added to extern/

This is because those libraries are autoconf based, and compiling them statically on linux and building them on Win/OSX is rather PITA for platform maintainers. Being covered with Blender's CMake will simplify maintenance a lot.

Parts of the libraries which are not used can simply be stripped out.

Proper CMake integration would need to be done, supporting:

a. Disabling of raqm and dependencies completely
b. Using system versions of raqm/fribidi/harfbuzz

  1. Fix bugs and make code stable
  2. Resolve FreeType reloading type
  3. All this should be optional in CMake, enabled for the full release builds.

Update: NOW you can build libraqm natively on windows or OSX using cmake, this can be done using libraqm-cmake port here.

Thanks for the update, from a quick look though it seems there are remaining code-review issues to solve still.

Did you try using text-regression.blend, and look into a way to avoid loading the font into memory and freeing for every update?

Was this done, and only needed review?
Khaled, Mazoon, what happened with this thing?

@Jaggz H (jaggz) It's been a long time since I stopped working on this.
I think it used to work more or less but there are still little unsolved issues (according to the last comments), I personally am currently busy with other projects and have no time (or the skills) to resolve said issues.
Whoever is interested in continuing this work can check the Raqm library:[[ URL | https://github.com/HOST-Oman/libraqm ]]