-
Notifications
You must be signed in to change notification settings - Fork 104
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
Font weight #274
Font weight #274
Conversation
For info, we had some discussions in a KOReader issue: koreader/koreader#4174 I'm not sure yet what to think about your PR. |
No, will be used real regular font variant if exist. |
Well, I guess this will be annoying in some use cases like mine :/ But on the other hand, I most often find "Regular" fonts too thin, that I usually correct with Gamma. So, I may stick to Medium/500 with this PR. But there will always be some font that 500 will be too much, and I'll have to re-render it with 400... So, feels like it could be helpful, but could also be bothering :) |
Well, it depends on the implementation of the frontend, I did it so that it was visible when a synthetic font is used, I think you can do that too.
But it's good that the font |
Just add a setting to prefer EDIT: Which is what this does, actually, so, yeah, I don't see the issue ;). User gets more choices, but that's kind of the point and it's per-font, everybody's happy? |
Haven't looked at the code, but one question I have is: what happens for bold stuff when you choose something above 400 as "default"? Does it still go to 700, or does it attempt to add 300 (or less?) to the selected weight? |
It wouldn't be per-font - but per-document, like the font size, font kerning. |
@poire-z: Could probably whip up something modeled after the font-size widget on our end? Or change the colors of the squares if synthetic in ProgressWidget, or something ;). Or kill the option in the bottom menu and move to the top menu near the font selection ;). |
Right, that's more or less what I was thinking of. If you're playing with fonts, you're playing with fonts, that just adds another aspect to it ;). |
TL;DR: I think this is great, and I'm much less worried about the frontend integration than @poire-z ;p. |
Was also wondering if there should be stuff tweaked elsewhere, like if "font-weight: bold" should add 300 instead of But it looks like we set 600 below for bold, not 700 - and 700 was picked because it was the nearest - now, a semibold 600 will be synthetized from the 700 ? coolreader/crengine/src/lvrend.cpp Lines 2252 to 2264 in 54c8ae2
All the DOM / CSS stuff doesn't need to know the fonts will be tweaked: coolreader/crengine/src/lvrend.cpp Lines 9494 to 9520 in 54c8ae2
I'm not that worried about it, I know we could have some fun finding a UI solution for that. |
// When using Harfbuzz, which uses itself the font metrics, that we | ||
// can't tweak at all from outside, we'll get positioning based on | ||
// the not-bolded font. We can't increase them as that would totally | ||
// mess HB work. |
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.
(Still in the process of updating all this for KOReader - posting some comments as a I read your stuff, before I can actually see how this does).
Are you sure this is all fine with Harfbuzz ? adding some advance just like that? How does it do with arabic cursive, diacritics, etc... ?
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 sure this is all fine with Harfbuzz ? adding some advance just like that?
No, I unsure.
How does it do with arabic cursive, diacritics, etc... ?
I haven't tested this at all. Somehow I didn't think about it.
if (glyph->blackBoxX == 0) // If a glyph has no blackbox (a spacing | ||
glyph->rsb = 0; // character), there is no bearing | ||
else | ||
glyph->rsb = (lInt16)(FONT_METRIC_TO_PX( (myabs(_slot->metrics.horiAdvance) | ||
glyph->rsb = (lInt16)(FONT_METRIC_TO_PX( (myabs(_slot->metrics.horiAdvance + _synth_weight_advance_add) |
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 have doubts about this synth_weight_advance_add as mentionned above - but this feels a bit quicky'n'hacky putting it all on the right side bearing.
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.
To be honest, I have not studied what it is at all, added by analogy. Think you need to remove it?
For the arabic/cursive/diacritics, did I gave you some test-scripts-my.html / test-scripts-my2.html ? There are hebrew characters with 2 or 3 diacritics in there. There is also some versions of them in bold, that I had tested with synthetized bold. (I still haven't tested this PR :) About the whole things with advances/RSB, I stick to some comments I wrote (and that you removed :): // When not using Harfbuzz, we will simply call FT_GlyphSlot_Embolden()
// to get the glyphinfo and glyph with synthetized bold and increased
// metrics, and everything should work naturally:
// When using Harfbuzz, which uses itself the font metrics, that we
// can't tweak at all from outside, we'll get positioning based on
// the not-bolded font. We can't increase them as that would totally
// mess HB work.
// Caveat: words in fake bold will be bolder, but not larger than
// the same word in the regular font (unlike with a real bold font
// were they would be bolder and larger). So, may be you could stick to not mess with advances with Harbuzz (and may be also with non-harfbuzz, if you want to avoid having to deal with diacritics and side bearings). Personally, I quite like how fake bold looks with Harfbuzz: no advance change, the bolder glyphs might make it feel like the font is a bit condensed, but it's quite pleasant. So, I guess I wouldn't mind having these +100/+200/-100 weight addition (smaller than current +300, or +200, emboldening) just do that. |
Yes, of course, me too :)
Ok, may be add option to enable/disable charcter width compensation for synthetic weight? I think this is a good feature, I would not want to give it up. |
I still don't want to have to build/install CoolReader :) And I need to see how this does on my device, with the fonts I use and the new ones I discovered and that I find too thin. Our emulator is fine for mostly everything else, but for fonts stuff, I need to be in ideal conditions :)
Well, I think we already have it: Harfbuzz vs non-Harfbuzz :) |
But
So if we move the addition of the _synth_weight_advance_add for synthetic weight here, it doesn't get any worse, right?
|
Yes, that's probably the best place to add it if we really want it. |
Yes i saw it: coolreader/crengine/src/private/lvfreetypeface.cpp Lines 1327 to 1332 in 54c8ae2
If nothing breaks, why not? |
Well, because I'm not wise/knowledgable enough to be sure that "nothing breaks" :) I ended the porting last evening, and played with it for an hour on my device (a bit annoying as I didn't do the UI stuff, I just made our
I'd like to test it without scaling with Harfbuzz, but not much time to give it more thoughts at the moment (I just migrated your code without much thinking). One idea that would at least help me and my use case (and the people who would know the trick) and stop my frustration would be a way to tell crengine to consider my font files with a specific weight instead of the one it guesses from the face name - like naming the file "Bitter Medium [cr-400].otf" so it considers it being 400 instead of 500 when loading it... (but that's just a hack, and won't work with .ttc collections...). I'd need @NiLuJe and @Frenzie to be able to play with that (once I have time to fix the italic bug, see about not scaling HB, and make the UI better) to get another opinion of how good or confusing this can be. I think it also needs a review pass around where we do bold - for normal fonts, it's 400 regular and 700 bold - but there are places when we do a +200 (or +201), actually going from 400 to 600 - and we may be then synthetizing a 600 bold instead of using the real 700 bold. |
@poire-z Ok, thank you. |
I mean, if it is already broken after adding letter spacing, then if we add some width compensation for the synthetic bold/weight, it will be more broken, or can we say that basically nothing has changed? :) |
But if a publisher uses I understand you want this scaling with Harfbuzz - I don't really want to stop you. It feels a bit adventurous to me - I'm rather conservative :) We already diverged a bit on the font side - so I'm fine if we take different decisions on this. |
Will give it a go tomorrow. |
Ditto ;). |
#274 (comment) updated files, including latest commit 32da167: |
The +5 is silly but it's legible, while the -3 is probably completely useless. I'd hide those (as well as -2 and +4) behind the little And as you already implied, between -1 and +2 you'd want half sizes or perhaps even more detail if possible. It's a bit unclear if hinting makes much of a difference. The fake bold also functions as fake hinting. Those opinions are pretty much universal across fonts, although in a font like Noto (which I normally wouldn't use for reading) the -2 is still a nicely legible thin while in most fonts it's already beyond the pale. And in Tex Gyre Pagella +4 is somehow less silly than in most. |
About hinting with synthetic outlines: These docs seem to say hinting is applied on an "outline" - so possibly correctly on the outline we have transformed (and not only on the original glyph outline, although I'd think "native" hinting, with its bytecode, should be proper to the font glyphs, and not some generic code that could hint anything after it's transformed...): And regarding my chronology: So, I'd say let's go with not disabling hinting, x3: if (_synth_weight > 0 || _italic == 2) { // Don't render yet
rend_flags &= ~FT_LOAD_RENDER;
- // Also disable any hinting, as it would be wrong after embolden
- rend_flags |= FT_LOAD_NO_AUTOHINT | FT_LOAD_NO_HINTING;
+ // Also disable any hinting, as it would be wrong after embolden.
+ // But it feels this is now fine after switching to FT_LOAD_TARGET_LIGHT.
+ // rend_flags |= FT_LOAD_NO_AUTOHINT | FT_LOAD_NO_HINTING; |
Some consistency issue: when using 550, depending on if we're coming from 500 or 600, a different font (the 400 or the 700) can be used to synthetize the 550. |
But we fixed it here: 4f4f1ea ... |
It's another issue. The commit you mentionned fixed wrongly using synthetic 500 or 600 to make 550. |
How can we decide what weight to take as a basis of 400 or 700, because everything ultimately depends on the font designer. One will say make 550 from 400 and the other from 700. How can we choose which is better? |
You have the same situation with synthetic 500 if the user has real 400 and a 600 fonts. |
@virxkane : I'm not really familiar with how CoolReader handles its settings.
which is fine for me. Am I right? |
It's to validate settings at program startup. |
Yes, we probably should give the user a predictable result for 550 - either always from 400, or always from 700, so that the result does not depend on the previous choice. |
OK, I'll look at it. (Bad idea to have included "Patch by @poire-z ..." in a commit title - I get a mail when each of the people who has cloned this buggins/coolreader syncs his repo :) Only 70 left :) |
For me it looks great at +1, the rest is not in the scope of my needs, I guess some rare cases would benefit of -1, but anyway I very much like what I see. (Reminds me of the densely printed Penguin paperbacks which seemed to use drooling amounts of ink so the letters' shapes were overly rounded and fat. And that's a memory dear to me, and to my astigmatic eyes ;) ) Some questions:
I like what this setting does more than what fontforge's auto weight transform made of fonts, but maybe it's just because of the ease of use :) Anyway there are a lot of OFL fonts I dropped due to being too thin that I would now reincorporcate into my 25 page long collection on Kobo ;) Thanks! |
Didn't know it worked like that. Well it's too late now. |
I also get this nostalgic feeling. Not that I ever read Penguin paperbacks, but some paper books I had also had inky fat glyphs - sometimes also like if the metal glyphs were a bit loose in the printer, not pefectly vertically aligned on the baseline - that I find again with syntethic weights and a "o" or "b" getting lower than a "n" or "k" :)
Yes. In my patch (it wasn't in this PR), hinting works on synthetic weights. Their effects depend on the font (although "auto" seems to indeed make glyphs taller). It was previously disabled because their effect feels like it would be unpredictable. Do you feel it's good to allow it? Or is "none" always the best?
Yes, it will be - with a fine tuning dialog allowing 0.25 steps.
You are correct. Thanks for the good feedback. |
Sorry gentlemens, but maybe here we will discuss how it looks in CoolReader? :) |
Yep, not my use-case either, but it's a fantastic addition, kudos @virxkane and everyone involved in the testing ;). |
@poire-z |
Solution I've found at koreader/crengine@207fc79 |
OK, thanks you. |
OK, experiment using 26.6 units is considered unsuccessful. |
I don't see myself here https://github.com/koreader/koreader/releases/tag/v2021.05 |
My bad, added. ;-) |
@Frenzie Thank you. |
Screenshots:
![NotoSerif-main-s](https://user-images.githubusercontent.com/36960933/112762080-06ec7100-900f-11eb-9e71-9601dd1fa6d9.png)
![NotoSerif-weights-s](https://user-images.githubusercontent.com/36960933/112762081-094ecb00-900f-11eb-882d-762d69c14583.png)
Font 'Noto Serif' full set of weights:
Font 'Coming Soon' (only Regular):
![ComingSoon-main-s](https://user-images.githubusercontent.com/36960933/112762092-12d83300-900f-11eb-9981-431d31c4f614.png)
![ComingSoon-weights-s](https://user-images.githubusercontent.com/36960933/112762093-14a1f680-900f-11eb-8ece-3490eb94ede0.png)
Book view with 'Coming Soon' regular:
![ComingSoon-rv-Regular](https://user-images.githubusercontent.com/36960933/112761968-bc6af480-900e-11eb-98f0-0a05c985a2bc.png)
Book view with 'Coming Soon' ExtraBold (fake/synthetic):
![ComingSoon-rv-ExtraBold(synth)](https://user-images.githubusercontent.com/36960933/112761972-bf65e500-900e-11eb-981e-2ec5ac4bf4f2.png)
Updated (20200330): added screenshots of synthetic weight for some arabic/hebrew text:
![NotoSansArabicUI Regular](https://user-images.githubusercontent.com/36960933/113024791-34b0f180-9198-11eb-9e18-92aaeeca9d3d.png)
![NotoSansArabicUI Regular-2](https://user-images.githubusercontent.com/36960933/113024872-4e523900-9198-11eb-84bf-c1f3dd72394b.png)
![NotoSansArabicUI Black (real)](https://user-images.githubusercontent.com/36960933/113024897-57430a80-9198-11eb-9138-829bf231cac3.png)
![NotoSansArabicUI Black (real)-2](https://user-images.githubusercontent.com/36960933/113024917-5d38eb80-9198-11eb-95fe-a1630ff0fe47.png)
![NotoSansArabicUI Black (synth)](https://user-images.githubusercontent.com/36960933/113024920-5e6a1880-9198-11eb-918a-499e2f4572e0.png)
![NotoSansArabicUI Black (synth)-2](https://user-images.githubusercontent.com/36960933/113024925-60cc7280-9198-11eb-9a18-61ea98faf394.png)
![NotoSansArabicUI (real, chromium)](https://user-images.githubusercontent.com/36960933/113032384-e05e3f80-91a0-11eb-9005-e51139e6fe77.png)
NotoSans + NotoSans Arabic UI Regular:
NotoSans + NotoSans Arabic UI Black (real):
NotoSans + NotoSans Arabic UI Black (both synth):
NotoSans Arabic UI Black (real) in Chromium: