-
Notifications
You must be signed in to change notification settings - Fork 184
Setting font features and font variations. (#503) #506
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
base: main
Are you sure you want to change the base?
Conversation
|
@Martin-Eriksson, I'd like to wrap up an SDL_ttf release, what's the status of this PR? We can definitely add these functions later, but if you'd like to get them in, I'm fine with that. |
|
I noticed some compiler warnings when testing this PR with GCC. I've fixed these warnings in my fork at: Feel free to incorporate these fixes into this PR if you find them useful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you still working on this? If not, can I take this over?
Also what were you doing when ASan threw the error, and what did ASan print?
| //naming? it's not from FreeType so maybe FT_ is inappropriate? | ||
| typedef struct FT_Variations { | ||
| int len; | ||
| FT_ULong *tags; | ||
| FT_Fixed *defaults; | ||
| FT_Fixed *coords; | ||
| } FT_Variations; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just name it variations as this is an internal struct.
| #if TTF_USE_HARFBUZZ | ||
| return font->features; | ||
| #else | ||
| (void) features; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is unnecessary.
| // The documentation (https://freetype.org/freetype2/docs/reference/ft2-multiple_masters.html#ft_mm_var) | ||
| // talks about named styles, like how 'bold' could be defined as [Weight=1.5,Width=1.1]. | ||
| // is that something we should care about? I don't know if HarfBuzz even parses that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is hb_set_var_named_instance() and hb_font_get_var_named_instance() for dealing with these. If you want the user to specify names instead of numbers, you can refer to hb-ot-var.
Like HarfBuzz and FreeType, you should probably add a separate set of functions for dealing with named instances.
| #if 0 | ||
| //! hb_font_set_variations does nothing | ||
| //! hb_font_set_variation doesn't work either | ||
| // is it supposed to work? Is it a bug in HarfBuzz or does it not work | ||
| // because we're using FreeType? | ||
| hb_font_set_variations(font->hb_font, font->variations, font->variations_len); | ||
| #else | ||
| // should we care if this returns an error? what should we do in that case? | ||
| FT_Set_Var_Design_Coordinates(font->face, font->ft_variations.len, font->ft_variations.coords); | ||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try calling hb_ft_hb_font_changed() after hb_font_set_variations()?
While you no longer need to call it in HarfBuzz 11.0.0 or later, not all distributions ship this new a HarfBuzz.
| #if 0 | |
| //! hb_font_set_variations does nothing | |
| //! hb_font_set_variation doesn't work either | |
| // is it supposed to work? Is it a bug in HarfBuzz or does it not work | |
| // because we're using FreeType? | |
| hb_font_set_variations(font->hb_font, font->variations, font->variations_len); | |
| #else | |
| // should we care if this returns an error? what should we do in that case? | |
| FT_Set_Var_Design_Coordinates(font->face, font->ft_variations.len, font->ft_variations.coords); | |
| #endif | |
| hb_font_set_variations(font->hb_font, font->variations, font->variations_len); | |
| hb_ft_hb_font_changed(font->hb_font); |
This commit adds the functionality to directly set font variation settings and font features. The work directly builds upon a prior attempt from libsdl-org#506. Instead of parsing a single string, the API is changed to handle arrays for easier maintenance. This also allows the font variation to be set without requiring HarfBuzz. Fixes: libsdl-org#503 Closes: libsdl-org#506 Co-authored-by: Martin Eriksson <[email protected]> Co-authored-by: Robert Sundling <[email protected]> Signed-off-by: Du Yijie <[email protected]>
This commit adds the functionality to directly set font variation settings and font features. The work directly builds upon a prior attempt from libsdl-org#506. Instead of parsing a single string, the API is changed to handle arrays for easier maintenance. This allows easier maintenance, and also allows font variations to be set without requiring HarfBuzz as in the prior attempt. Fixes: libsdl-org#503 Closes: libsdl-org#506 Co-authored-by: Martin Eriksson <[email protected]> Co-authored-by: Robert Sundling <[email protected]> Signed-off-by: Du Yijie <[email protected]>
This commit adds the functionality to directly set font variation settings and font features. The work directly builds upon a prior attempt from libsdl-org#506. Instead of parsing a single string, the API is changed to handle arrays for easier maintenance. This allows easier maintenance, and also allows font variations to be set without requiring HarfBuzz as in the prior attempt. Fixes: libsdl-org#503 Closes: libsdl-org#506 Co-authored-by: Martin Eriksson <[email protected]> Co-authored-by: Robert Sundling <[email protected]> Signed-off-by: Du Yijie <[email protected]>
This commit adds the functionality to directly set font variation settings. The work directly builds upon a prior attempt from libsdl-org#506. Instead of parsing a single string, the API is changed to expect an array of variation settings, in the same format expected by the HarfBuzz function hb_font_set_variations(). This allows the font variation to be set even when HarfBuzz is not used. Link: libsdl-org#503 Link: libsdl-org#506 Co-authored-by: Martin Eriksson <[email protected]> Co-authored-by: Robert Sundling <[email protected]> Signed-off-by: Du Yijie <[email protected]>
This commit adds the functionality to directly set font features. The work directly builds upon a prior attempt from libsdl-org#506. Instead of parsing a single string, the API is changed to expect an array of strings. OpenType features can be applied not only to everything using the font, but also to only a part of some text. This commit does not address this. Link: libsdl-org#503 Link: libsdl-org#506 Co-authored-by: Martin Eriksson <[email protected]> Co-authored-by: Robert Sundling <[email protected]> Signed-off-by: Du Yijie <[email protected]>
This commit adds the functionality to directly set font variation settings. The work directly builds upon a prior attempt from libsdl-org#506. Instead of parsing a single string, the API is changed to expect an array of variation settings, in the same format expected by the HarfBuzz function hb_font_set_variations(). This allows the font variation to be set even when HarfBuzz is not used. Link: libsdl-org#503 Link: libsdl-org#506 Co-authored-by: Martin Eriksson <[email protected]> Co-authored-by: Robert Sundling <[email protected]> Signed-off-by: Du Yijie <[email protected]>
This commit adds the functionality to directly set font features. The work directly builds upon a prior attempt from libsdl-org#506. Instead of parsing a single string, the API is changed to expect an array of features to apply globally, similar to the font variation support implementation. The option to apply a feature to a specific part of every text using the font is no longer present. OpenType features can be applied not only to everything using the font, but also to only a part of some text. This commit does not address this. Link: libsdl-org#503 Link: libsdl-org#506 Co-authored-by: Martin Eriksson <[email protected]> Co-authored-by: Robert Sundling <[email protected]> Signed-off-by: Du Yijie <[email protected]>
Closes #503.
I've got some comments in there with questions about how to do certain things.
I had thought I could use
hb_font_set_variationsand it would be super easy,barely an inconvenience, to support setting variations but it does nothing at all.
Instead I had to use FreeType directly, which took me a while to figure out how
to do. Do you think
hb_font_set_variationsis supposed to work? Or is it correctthat it does nothing when using FreeType, perhaps?
font->enable_kerningis not used at all anymore,TTF_SetFontFeaturesandTTF_SetFontKerningare completely independent. It ignores the
font->enable_kerningvalueif
TTF_SetFontFeatureshas been called with a non-null string. IfTTF_SetFontFeaturesis called later with a null string, it starts using the
font->enable_kerningvalue again.Combining them is a bit complicated if we want to avoid doing more work in
CollectGlyphsFromFont().I've tried to run address sanitizer on the code but I haven't got it working yet.
It reports nothing even though I've commented out calls to free.
Until I've figured out how to get ASan working, and if
hb_font_set_variationsis supposed to work, I'm keeping this asa draft PR