-
Notifications
You must be signed in to change notification settings - Fork 333
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
Upgraded font-weight #296
Upgraded font-weight #296
Conversation
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.
Thank you! This is awesome, a very welcome addition.
I have some suggestions here after looking over it, let me know what you think.
Haven't tested it properly yet, I see the PR is failing many of the unit tests. Let me know if I can be of help for fixing them.
I've made a commit with the requested code changes. |
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.
Thanks for continuing to work on this!
Can I ask about some use cases:
- How do we handle the case when the font face does not contain the desired weight? Do we load it at all?
- If we still load it, what should the font weight be registered as?
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.
I tested this PR more thoroughly now. Appreciate your continued work on this! Sorry about my reviews coming a bit in chunks, I have gone over everything now though.
Currently, the samples don't open correctly since all included fonts fail to load. It seems to fail whenever the font file does not provide anything for FT_Get_MM_Var
, which seems common for all single-weight fonts. A quick fix is to continue after GetFaceInstanceIndexForWeight
regardless, when the weight is auto. However, this is not sufficient for all cases. See the following test code for loading fonts in the sample shell:
void Shell::LoadFonts(const char* directory)
{
using Rml::Style::FontWeight;
struct FontFace {
Rml::String filename;
bool fallback_face;
FontWeight weight;
};
FontFace font_faces[] = {
{ "OpenSans-VariableWidth.ttf", false, FontWeight::Auto }, // Loads weights [300, 400, 600, 700, 800, 300, 400, 600, 700, 800] (loads faces twice!)
{ "OpenSans-VariableWidth.ttf", false, FontWeight(800) }, // Loads 800 (correct)
{ "OpenSans-VariableWidth.ttf", false, FontWeight::Light }, // Loads 300 (correct)
{ "OpenSans-Regular.ttf", false, FontWeight::Auto }, // Fails to load, with basic fix it loads as 400 (correct)
{ "OpenSans-Bold.ttf", false, FontWeight::Auto }, // Fails to load, with basic fix it loads as 700 (correct)
{ "OpenSans-ExtraBold.ttf", false, FontWeight::Auto }, // Fails to load, with basic fix it loads as 400 (wrong)
{ "OpenSans-Light.ttf", false, FontWeight::Auto }, // Fails to load, with basic fix it loads as 400 (wrong)
{ "OpenSans-Regular.ttf", false, FontWeight::Normal }, // Fails to load (ideally this would load correctly and be registered as 400)
{ "OpenSans-Bold.ttf", false, FontWeight::Bold }, // Fails to load (ideally this would load correctly and be registered as 700)
{ "OpenSans-ExtraBold.ttf", false, FontWeight::Bold }, // Fails to load (ideally this would load correctly and be registered as 700)
{ "OpenSans-ExtraBold.ttf", false, FontWeight::Black }, // Fails to load (ideally this would load correctly and be registered as 900)
{ "LatoLatin-Regular.ttf", false, FontWeight::Auto }, // Fails to load, with basic fix it loads as 400 (correct)
{ "LatoLatin-Italic.ttf", false, FontWeight::Auto }, // Fails to load, with basic fix it loads as 400 (correct)
{ "LatoLatin-Bold.ttf", false, FontWeight::Auto }, // Fails to load, with basic fix it loads as 700 (correct)
{ "LatoLatin-BoldItalic.ttf", false, FontWeight::Auto }, // Fails to load, with basic fix it loads as 700 (correct)
{ "NotoEmoji-Regular.ttf", true , FontWeight::Auto }, // Fails to load, with basic fix it loads as 400 (correct)
};
for (const FontFace& face : font_faces)
{
Rml::LoadFontFace(Rml::String(directory) + face.filename, face.fallback_face, face.weight);
}
}
What we want is the ability to override the font weight if it registers as the wrong one.
Thus, I propose this functionality for the weight_to_load
parameter:
- If the font provides multiple font weights, then the weight parameter works like now: Loads only the appropriate weight and fails if it cannot be found.
- On the other hand, if the font face contains only a single face (whether or not it provides
MM_var
), then the weight parameter overrides whatever is auto-detected. - This would also ensure backward compatibility, currently it fails loading the debugger plugin for this reason.
If you agree, we should probably change the weight_to_load
parameter back to weight
again, since it now has two meanings, one of them not having anything to do with loading. This behavior should be noted in the function calls in Core.h
.
Hey. I'm just wondering if you intend to keep working on this PR? We are getting close now, but otherwise I can offer to fix up the last bits so we can get it merged. Cheers! |
Hey, apologies for not finishing the code changes, I've been very busy recently. Thanks for offering to finish them, that would be awesome if you could. |
No worries! I'll give it a go then. |
Hey! I fixed some of the issues and changes discussed, in addition to some general refactoring. See the commit messages for details. Could you test this on your end? Let me know if you have any suggestions for improvements. |
…g fonts with multiple font-weights. updated font-weight fixes for different os compilers Requested changes. small fix accidently left an include line in requested changes 2 Add additional named weights to FontWeight, but only allow 'normal' and 'bold' in RCSS for compatibility with CSS. Make memory for font faces owned by their font family, prevents use-after-free. Minor comment Change weight_to_load parameter back to weight Font-weights refactoring and additions: - Font files with a single face now loads correctly again. - Only a single face will be loaded for a given font-weight value even when there are multiple width variations (choosing the width closest to 'regular' width). - Slightly changed the purpose of the 'weight' parameter, it should now be backwards compatible with previous usage. - No longer need to search the font variations twice, passing in the named style to the load face function. Add missing header Replace custom parser for the 'font-weight' property with generic parsers Remove non-standard enum values from FontWeight Co-authored-by: Michael Ragazzon <michael.ragazzon@gmail.com>
This PR upgrades support for font-weight in .rcss files:
font-weight: (uint:0-0xFFFF)
and
font-weight: thin|light|normal|medium|bold|black
The PR supports 2 ways to load weights for fonts:
Rml::LoadFontFace("assets/OpenSans.ttf", false, 300); // Only load weight 300 from this font file.
or
Rml::LoadFontFace("assets/OpenSans.ttf", false); // Load all weights from this font file. (The third arg defaults to 0).
The PR also tries to match faces by the nearest weight, after trying to match by a direct weight.