Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Better Text Rendering for cairo / lua #1501

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

simotek
Copy link
Contributor

@simotek simotek commented Apr 12, 2023

Description

Introduction

I have attempted to resolve #900 by creating a new lua binding specifically for doing font rendering in Cairo using Freetype and Harfbuzz, fontconfig is also used to locate the correct fonts. I have chosen to follow the approach of the imlib2_helper binding and not expose functions from the headers of each of these libraries but rather provide simple functions that "internally" use each of these libraries.

At the time of creating this PR I have no fixed thoughts on the API other then it works for my use case and tries to balance the simple act of rendering an English string while also allowing complex options such as RTL text as requested in #75 as well as being able to center and right align text #940 and #1017. So your thoughts on the API topic would be much appreciated, once the API is finalized I will add appropriate documentation. currently this is the last major step for be before being able to move to a lua only config.

Current API Description (Happy to make changes here)

Loading a font from a file is a slow enough operation that we probably don't want to do it multiple times so I separated loading fonts and freeing memory at close into separate functions.

/* font is a string containing the name of the font to load */
/* this will be matched on a best effort basis and FontConfig will fall back to a similar font if the specified font isn't found */
FontData * cairo_text_hp_load_font(const char *font, int font_size);
void cairo_text_hp_destroy_font(FontData *font);

The next set of functions cover the "Simple" English use cases, I could have added a parameter for alignment but I was trying to keep the common Left case as simple as possible.

void cairo_text_hp_simple_show(cairo_t *cr, int x, int y, const char *text, FontData *font);
void cairo_text_hp_simple_show_center(cairo_t *cr, int x, int y, const char *text, FontData *font);
void cairo_text_hp_simple_show_right(cairo_t *cr, int x, int y, const char *text, FontData *font);

The next function is the "catch all" with the main implementation and allows tweeking of the internalization options such as text direction in Harfbuzz for people looking to use non latin languages.

/* 
 * Direction calls hb_direction_from_string example values are LTR and RTL
 *   https://harfbuzz.github.io/harfbuzz-hb-common.html#hb-direction-from-string
 * Script is an ISO 15924 4 character string, "Zyyy" can be used for "Common" and "Zinh"
 *  for "Inherited". 
 *  https://harfbuzz.github.io/harfbuzz-hb-common.html#hb-script-from-string
 * Language is a BCP 47 language tag. eg "en" or "en-US"
 */
void cairo_text_hp_intl_show(cairo_t *cr, int x, int y, cairo_text_alignment_t alignment, const char *text, 
                                                    FontData *font,  const char *direction, const char *script, const char *language);

Finally when doing layouting sometimes its useful to know how wide the final string will be so the following find the text width, again there is a simple latin version and an internationalized version. In the future I'll also provide a Height function which will be important for supporting vertical text.

int cairo_text_hp_simple_text_width(const char *text, FontData *font);
int cairo_text_hp_intl_text_width(const char *text, FontData *font, const char *direction, const char *script, const char *language);

Screenshot

In the following screenshot the "Cairo" text is using Cairo's built in basic text rendering, the "Conky" text is using Conky's standard text rendering and the "Freetype" strings are using this new API with Freetype and Harfbuzz rendering the text in Cairo. All these cases are just using the default settings as found on openSUSE Tumbleweed.

shot-2023-04-11_21-13-49

simotek and others added 3 commits April 11, 2023 21:35
Add empty files and various build options for the lua text bindings
Adds requirements for Fontconfig, Freetype and Harfbuzz
@netlify
Copy link

netlify bot commented Apr 12, 2023

Deploy Preview for conkyweb canceled.

Name Link
🔨 Latest commit d5d20e8
🔍 Latest deploy log https://app.netlify.com/sites/conkyweb/deploys/643606d387ba2c0007c0b550

@github-actions github-actions bot added appimage Issue or PR that's related to AppImage changes gh-actions Issue or PR that suggest changing GitHub actions sources PR modifies project sources labels Apr 12, 2023
I thought I already removed this and squashed the commit
Copy link
Owner

@brndnmtthws brndnmtthws left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great overall, just need to add documentation to https://github.com/brndnmtthws/conky/blob/main/doc/lua.yaml for the new functions.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could probably enable this in the macos build too, if you want to take a stab at that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once I figure out why the ubuntu builds are failing i'll have a shot at enabling it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The functions in here to be documented in conky/doc/lua.yml.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've had a shot at doing this in a way that hopefully makes sense, for some reason i couldn't generate docs locally but that doesn't seem related to any of my changes.

If your happy with that i'll probably do similar for the imlib2 bindings that also aren't currently documented.

@simotek
Copy link
Contributor Author

simotek commented Apr 17, 2023

Thanks for the feedback, i'll update the docs and finish of implementing the Top to Bottom features then finish up a couple of configs to use them before sending through some updates. But its good to know i'm heading in the right direction.

@moceap
Copy link

moceap commented Jul 4, 2023

After trying this patches, bug #75 still exist!

Screenshot_٢٠٢٣٠٧٠٤_٠٣٢٨٤١

@simotek
Copy link
Contributor Author

simotek commented Jul 6, 2023

After trying this patches, bug #75 still exist!

Screenshot_٢٠٢٣٠٧٠٤_٠٣٢٨٤١

Yes that is currently to be expected, the WIP patch just had the LTR support so I could get feedback on the API, RTL is almost there I just need to finish writing the tests.

Also this change only works with the lua API, however it is the first step in my project to add a toolkit to make using the lua API much simpler. Although that is further from completion.

@simotek
Copy link
Contributor Author

simotek commented Jul 20, 2023

@moceap Can you confirm that the following screenshot looks ok, it should be the days of the week.
shot-2023-07-20_15-27-09

I'll probably do just a little more testing of my actual real world usecases before sending through an updated pull request

@simotek
Copy link
Contributor Author

simotek commented Jul 20, 2023

shot-2023-07-20_15-51-45

This one uses Noto Sans Arabic instead.

@moceap
Copy link

moceap commented Aug 9, 2023

No it's not OK.

Letters in Arabic should be connected:

You write:

الإثنين
يوم الثلاثاء
الأربعاء
يوم الخميس
جمعة
السبت
الأحد

Look at them here in the browser, they are connected.

@simotek
Copy link
Contributor Author

simotek commented Aug 10, 2023

No it's not OK.

Letters in Arabic should be connected:

You write:

الإثنين يوم الثلاثاء الأربعاء يوم الخميس جمعة السبت الأحد

Look at them here in the browser, they are connected.

Thanks for the confirmation, that's what I thought i'll have to have a closer look at whats going wrong, the left to right seems to be ok.

Copy link
Collaborator

@Caellian Caellian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never used HarfBuzz, but from what I picked up googling it, the way you're using it seems fine.

//alternatively you can use hb_buffer_set_unicode_funcs(buf, hb_glib_get_unicode_funcs());
hb_buffer_set_unicode_funcs(buf, hb_glib_get_unicode_funcs());
hb_buffer_set_direction(buf, text_direction); /* or LTR */
hb_buffer_set_script(buf, text_script); /* see hb-unicode.h */
Copy link
Collaborator

@Caellian Caellian Nov 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which text_script are you using to test out arabic? It should be "Arab" as defined in hb-common.h.

}
hb_script_t text_script = hb_script_from_string(script, -1);
if (text_script == HB_SCRIPT_UNKNOWN) {
text_script = HB_SCRIPT_COMMON;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try setting this to HB_SCRIPT_ARABIC (string literal) instead of HB_SCRIPT_COMMON and see if it works (though it will break everything else).

//alternatively you can use hb_buffer_set_unicode_funcs(buf, hb_glib_get_unicode_funcs());
hb_buffer_set_unicode_funcs(buf, hb_glib_get_unicode_funcs());
hb_buffer_set_direction(buf, text_direction); /* or LTR */
hb_buffer_set_script(buf, text_script); /* see hb-unicode.h */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be hb-common.h as well. Just pointing it out (so it's easier for others to pick up), not that these comments will affect anything.

text_script = HB_SCRIPT_COMMON;
}

hb_language_t text_language = hb_language_from_string (language, -1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the language set to "ar"?

Copy link
Contributor Author

@simotek simotek Dec 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, the full setting was {font_family="Noto Sans Arabic", font_size=16, align=CAIRO_TEXT_ALIGN_LEFT, font_direction="RTL", font_script="HB_SCRIPT_ARABIC", font_language="ar"}

hb_buffer_destroy(buf);
}

void cairo_text_hp_simple_show(cairo_t *cr, int x, int y, const char *text, FontData *font)
Copy link
Collaborator

@Caellian Caellian Nov 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that there's no need for these cairo_text_hp_simple_show_* convenience functions. toluapp supports default values and harfbuzz can infer defaults too.

You can make all of the properties optional by:

  • adding a call to hb_buffer_guess_segment_properties (hb_buffer); in cairo_text_hp_intl_show after specified properties are set.
  • setting argument order to be: language (because it's inferred from system (least smart)), script (inferred from buffer contents), direction (inferred from script)
    • as documented here
    • that order would minimize the need for supplying well inferred values to set the one without a good default
  • specifying argument defaults to be nullptr/NULL in the header
    • e.g. const char *direction = NULL
    • and not calling hb_*_from_string if the value is null (i.e. empty string).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the same line of reasoning I'd position cairo alignment argument after the font argument and set it to default to CAIRO_TEXT_ALIGN_LEFT.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the binding function should be able to load and cache a font. The cache can leak memory (no need to call free) as it would be weird to manage such things from a lua script and fonts will likely be needed for each draw call.

So the signature of cairo_text_hp_intl_show in .pkg would finally look like: void cairo_text_hp_intl_show(cairo_t *cr, int x, int y, const char *text, const char *font = NULL, int font_size = 12, cairo_text_alignment_t alignment = CAIRO_TEXT_ALIGN_LEFT, const char *language = NULL, const char *script = NULL, const char *direction = NULL).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I didn't see this until now, I'll certainly swap to the idea of default arguments that seems much better. Leaking fonts seems reasonable to me as well so i'm happy to just add a cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am implementing most of this, with the exception of language which i'm going to default to english if not explicitly set. The reason being if I create a config / script for conky and want to distribute it, I don't want it to start breaking in random ways because other people are using a different language. Also much of conky's outputs such as process names CPU percentages etc work best in English. In this case if you are doing something that requires an explicit language you'll need to set it as well.

@moceap
Copy link

moceap commented Dec 4, 2023

@simotek Take a look https://github.com/HOST-Oman/libraqm, It may fast the process.

@simotek
Copy link
Contributor Author

simotek commented Dec 8, 2023

@simotek Take a look https://github.com/HOST-Oman/libraqm, It may fast the process.

One thing I noticed at https://github.com/HOST-Oman/libraqm/blob/9d7ca9d59dba38e777bdcd096870776dab5e6e66/src/raqm.c#L1793C1-L1793C48 is that for the RTL case they seem to have hardcoded a -1 into the calculation for character position, So I might have to have a play with that later.

It seemed like hardcoding a -1 into the width of each character would solve the issue but I didn't want to just do it without seeing it documented, but the fact this other codebase is suggests its probably the right solution

simotek and others added 9 commits February 14, 2024 10:19
Add empty files and various build options for the lua text bindings
Adds requirements for Fontconfig, Freetype and Harfbuzz
This should go into its own PR but right now I need it for my system
to work.
This replaces the old approach of using a series of function
declarations.
For some reason RTL positioning is wrong, it seems somewhat based
on font size 1/10th font size seems to get closer to correct so
run with that for now, i'll continue investigating a proper fix.
Copy link

netlify bot commented Feb 15, 2024

Deploy Preview for conkyweb ready!

Name Link
🔨 Latest commit f9676c6
🔍 Latest deploy log https://app.netlify.com/sites/conkyweb/deploys/67186728c6a0f900084f6fa0
😎 Deploy Preview https://deploy-preview-1501--conkyweb.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@simotek
Copy link
Contributor Author

simotek commented Feb 15, 2024

So far I've done all the interface changes, other then caching fonts, which is next on my list.

As for the RTL codepaths, subtracting 1/10th of the font size seems to get much closer then -1 especially for larger font sizes. Its not perfect for all fonts at all sizes though, but is significantly better, i'll follow this up with upstream closer

@moceap I presume this screenshot is getting much closer. At this point I have no idea if the text is actual words.

shot-2024-02-15_11-53-40

@simotek
Copy link
Contributor Author

simotek commented Feb 15, 2024

Also if anyone has any idea what Ubuntu packages are needed to get cmake to find freetype that would be much appreciated so that I don't need to spin up a VM and test.

Fonts are now cached in C, given the number of fonts and sizes
I expect people will use, this implementation seems simpler then
implementing hashmaps or attempting to do bindings in C++
This reverts commit 5036cc4.
Fixed better upstream now
On my machine i'm using it with lua5.3 and I see packages for
5.4 as well.
@github-actions github-actions bot added the documentation Issue or PR that suggests documentation improvements label Feb 22, 2024
Copy link
Collaborator

@Caellian Caellian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some inputs on tables in docs.

| script | A string containing a ISO 15924 script tag. |
| | Default value: auto-detect from text |
| direction | A string representing text direction eg `LTR`, `RTL` or `TTB` |
| | Default value: auto-detect from text otherwise `LTR` | |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra pipe (|) at the end.

| script | A string containing a ISO 15924 script tag. |
| | Default value: auto-detect from text |
| direction | A string representing text direction eg `LTR`, `RTL` or `TTB` |
| | Default value: auto-detect from text otherwise `LTR` |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of putting default value into the next row, it might be better to put it in the same cell and use <br/> so that it looks like this:

Argument Description
language A string containing a BCP 47 language tag.
Default value: en

@Caellian
Copy link
Collaborator

Also if anyone has any idea what Ubuntu packages are needed to get cmake to find freetype that would be much appreciated so that I don't need to spin up a VM and test.

Should be libfreetype6-dev, see: https://askubuntu.com/questions/1347131/what-is-the-reason-of-cmake-error-when-using-freetype2-header

  • CMake is crappy at locating dependencies sometimes (amongst a rather long list of other things), if you have that package installed, send the output of apt-file list libfreetype6-dev so we can patch the Find script.

@github-actions github-actions bot added dependencies Issue or PR that affects dependencies build system Issues and PRs related to build system (CMake) and process lua Issue or PR related to Lua code cairo Issue or PR related to cairo labels Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
appimage Issue or PR that's related to AppImage changes build system Issues and PRs related to build system (CMake) and process cairo Issue or PR related to cairo dependencies Issue or PR that affects dependencies documentation Issue or PR that suggests documentation improvements gh-actions Issue or PR that suggest changing GitHub actions lua Issue or PR related to Lua code sources PR modifies project sources
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can I use the Imlib2 Lua bindings for font rendering?
4 participants