Allow selecting fonts from collections

[leduyquang753]

This pull request adds a face_index parameter to the LoadFontFace function to allow selection of font faces within font collections.

User impact: no impact when LoadFontFace is called with just a single path argument as the face_index argument is defaulted to 0, however face_index will need to be specified if further arguments are used like fallback_font.

[mikke89]

Hi, and thanks a lot for the pull request! It's a solid effort, and I am happy to see that you are fixing up the samples and plugins as well.

I don't know how common such font collections are, but I'm sure if you need it then someone else might too :)

I feel like the LoadFontFace parameters are starting to get quite crowded. I think we should consider moving some of the parameters into a struct instead.

This is a breaking change that will most likely impact many users, so I don't want to merge it just now. I might hold off on merging it until a new major version of the library.

[szdytom]

I just want to mention a notable use case of the feature: font like Noto Sans CJK will pack all its Language/Region Specific versions (such as SC, TC, KR, JP, HK) in one file. Currently, RmlUI will always load the Japan variant (maybe because it is alphabetically the smallest).

[mikke89]

Ah, I see, that's a good motivation to add this feature indeed.

Would be great to see further updates to his PR. I think we can consider adding it in a minor RmlUi version if we can keep the Rml::LoadFontFace interface the same. I am more inclined to break the font engine API somewhat, as that affects significantly fewer users.

[leduyquang753]

I think we can consider adding it in a minor RmlUi version if we can keep the Rml::LoadFontFace interface the same.

A more backwards-compatible but less elegant solution would be to make a separate function called LoadFontFaceFromCollection or so.

[mikke89]

I did some more thinking on this. I feel like our current API is a bit weirdly divided into these two different signatures:

bool LoadFontFace(const String& file_path, bool fallback_face, Style::FontWeight weight);

bool LoadFontFace(Span<const byte> data, const String& family, Style::FontStyle style,
	Style::FontWeight weight, bool fallback_face);

The main distinction here is whether or not the font is loaded from file or from memory. However, in the second signature we also allow users to override some registered properties of the font. However, it doesn't really make sense for the rest of the parameters to be different, we might just as well allow that for the first signature as well.

And then, trying to classify these parameters, one attempt to group them is as follows:

1. Source (location or data)
2. Selection criteria (weight and now face index with this PR)
3. Properties/font description overrides (family, style, weight)
4. Font behavior (?) (fallback)

Also, there is a lot more we could potentially do in the future. We could just as well support variable font variations such as italic and width, like we currently do for font weight.

Actually, I'm not entirely sure if the distinction between those groups above are always that clear. We could also take inspiration from CSS @font-face. We also had this elaborate issue on composite fonts: #500, but hopefully we don't need to get into all of that here.

Trying to stay closer to the CSS at-rule, here is one suggestion, replacing both of the earlier signatures (but keeping them for backward compatibility):

struct FontFaceSource {
	String file_path;
	Span<const byte> data;
};
struct FontFaceDescriptors {
	String font_family;
	Style::FontStyle font_style = Style::FontStyle::Auto;
	Style::FontWeight font_weight = Style::FontWeight::Auto;
	int face_index = 0;
	bool fallback_face = false;
};
bool LoadFontFace(const FontFaceSource& source, const FontFaceDescriptor& descriptors);

Using a separate struct makes it a lot easier to extend in the future. Compared to the CSS rule, the face_index and fallback_face are the odd ones standing out here. We could consider pulling those out into a separate struct.

What do you think? Now of course, I understand this discussion isn't directly tied to your PR. But since we need to change API, I think we need to discuss these changes in this relation. Hope that seems reasonable.

[leduyquang753]

That does look much nicer, although FontFaceSource should probably be a tagged union (which would be std::variant but the library does not use C++ 2017).

About the suggestion of composite fonts, I think it could be solved by just allowing a series of names for font-family. This might go hand in hand with the idea of sharing a texture atlas for all fonts as we could then easily take glyphs from other fonts on the fly.

[rminderhoud]

Just wanted to chime in that a fix for this would be nice and I thought I would share my use case. Recently we downloaded some fonts from google fonts and they include a single font collection with variable weights, e.g.

NotoSansJP-VariableFont_wght.ttf

In addition to their "static" collection of fonts:

NotoSansJP-Bold.ttf
NotoSansJP-ExtraBold.ttf
NotoSansJP-ExtraLight.ttf
...

They strongly recommended the variable font. However this collection didn't work with RmlUi because only the first face is loaded. While being able to manually include extra faces inside the font collection is useful it also would be useful to have an option to load all of them. I think this is a pattern of font file that might become more common if it isn't already so.

Wanted to share in case it impacts the API design.

EDIT: After some reading it COULD have also been the issue mentioned here #500 as the font was used as a fallback font

[mikke89]

@rminderhoud I believed this was something we supported already, implemented by #296. Fonts are complicated, so please let us know if this doesn't work as intended. With that said, the control over the exact weight values are a bit limited. And it's a bit confusing since sometimes the weight argument means override the value, and other times it means load a particular weight.

Actually, I think we need to spend some more time re-thinking and designing this API. And I don't want to block this PR in the meantime.

So meanwhile, my suggestion is to keep it simple and non-API-breaking, by adding it as a last defaulted argument to each of our current functions:

bool LoadFontFace(const String& file_path, bool fallback_face = false,
    Style::FontWeight weight = Style::FontWeight::Auto, int face_index = 0);

bool LoadFontFace(Span<const byte> data, const String& family, Style::FontStyle style,
	Style::FontWeight weight = Style::FontWeight::Auto, bool fallback_face = false, int face_index = 0);

About the suggestion of composite fonts, I think it could be solved by just allowing a series of names for font-family. This might go hand in hand with the idea of sharing a texture atlas for all fonts as we could then easily take glyphs from other fonts on the fly.

Could you elaborate a bit on this idea? Based on the remarks by @rminderhoud, I was thinking perhaps we want to take a Vector<FontFaceDescriptors>, which could be used to load specific weights or other "variables" from that font face. I am not sure how different font families would play into this though.

[leduyquang753]

Could you elaborate a bit on this idea? Based on the remarks by rminderhoud, I was thinking perhaps we want to take a Vector<FontFaceDescriptors>, which could be used to load specific weights or other "variables" from that font face. I am not sure how different font families would play into this though.

Actually I am trying to address the font fallback problem that #500 originally mentioned. In the current implementation, as there is only one chain of fallback fonts, each font face currently gets away with directly taking fallback glyphs into its own texture atlas. When all font faces share one texture atlas, we could immediately make use of glyphs from other font faces as fallback, so in turn we could afford to have arbitrary fallback chains.

[mikke89]

Oh, I see, so it acts as a replacement for the fallback_font bool. That is definitely something to consider when enabling global a global font atlas, although I think that discussion should continue in #500.