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

Font weight #274

Merged
merged 14 commits into from Apr 5, 2021
Merged

Font weight #274

merged 14 commits into from Apr 5, 2021

Conversation

virxkane
Copy link
Collaborator

@virxkane virxkane commented Mar 28, 2021

  • When registering fonts, the font weight is determined by the style name. Previously, without this, some font variants were discarded as duplicates.
  • The software one-position embolding of the font in the absence of a bold version is replaced with multi-step change of the font weight. Also implemented compensation for under/over-width characters when setting fake/synthetic font weight. Document property 'PROP_FONT_BASE_WEIGHT' (font.face.base.weight) replaces 'PROP_FONT_WEIGHT_EMBOLDEN' (font.face.weight.embolden). Document command 'DCMD_SET_BASE_FONT_WEIGHT' replaces 'DCMD_TOGGLE_BOLD'.
  • Desktop/Qt5: frontend updated to be able to change font weight in settings.
  • Android: frontend updated to be able to change font weight in settings.

Screenshots:
Font 'Noto Serif' full set of weights:
NotoSerif-main-s
NotoSerif-weights-s

Font 'Coming Soon' (only Regular):
ComingSoon-main-s
ComingSoon-weights-s

Book view with 'Coming Soon' regular:
ComingSoon-rv-Regular

Book view with 'Coming Soon' ExtraBold (fake/synthetic):
ComingSoon-rv-ExtraBold(synth)

Updated (20200330): added screenshots of synthetic weight for some arabic/hebrew text:
NotoSans + NotoSans Arabic UI Regular:
NotoSansArabicUI Regular
NotoSansArabicUI Regular-2
NotoSans + NotoSans Arabic UI Black (real):
NotoSansArabicUI Black (real)
NotoSansArabicUI Black (real)-2
NotoSans + NotoSans Arabic UI Black (both synth):
NotoSansArabicUI Black (synth)
NotoSansArabicUI Black (synth)-2
NotoSans Arabic UI Black (real) in Chromium:
NotoSansArabicUI (real, chromium)

@poire-z
Copy link
Contributor

poire-z commented Mar 28, 2021

For info, we had some discussions in a KOReader issue: koreader/koreader#4174

I'm not sure yet what to think about your PR.
From a technical point, I guess it's just fine switching this boolean Embolden false/true to a set of values and have more granularity.
But I'm not sure how this will work in some situations, like mine :) Say I have as my main font Bitter Medium (because Bitter Regular is too thin) and Bitter Bold. Bitter Medium is currently picked and used as the regular one by crengine, and all is fine. Will this PR, with the default setting of "400" (that I'll keep as the default, so all other fonts get natural), will a regular 400 Bitter be synthetized from the 500 Medium I have ?

@virxkane
Copy link
Collaborator Author

virxkane commented Mar 28, 2021

Will this PR, with the default setting of "400" (that I'll keep as the default, so all other fonts get natural), will a regular 400 Bitter be synthetized from the 500 Medium I have ?

No, will be used real regular font variant if exist.
If "regular" variant of the font ommited - it will be synthesized from "medium", but, of course, you can select "medium" weight to excude this.

@poire-z
Copy link
Contributor

poire-z commented Mar 28, 2021

If "regular" variant of the font ommited - it will be synthesized from "medium", but, of course, you can select "medium" weight to excude this.

Well, I guess this will be annoying in some use cases like mine :/
"400" fonts can be thin or strong depending on the designer. I'm usually just fine using them as they are. When I see a Medium instance, I test it, and sometimes it's too weighty, sometimes just as fine to be used instead of the regular. So, I have a mixed of regular/medium (well, one of them by font) - that currently gets picked as they are.
With this PR, when I change fonts, I'll have then sometimes to adjust the weight to get their original weight instead of some synthetized weight I'm not used to see. And I may not even know it's synthetized or not (may be that doesn't matter, or does emboldening kills some intrinsic quality of the font?).

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 :)

@virxkane
Copy link
Collaborator Author

virxkane commented Mar 28, 2021

And I may not even know it's synthetized or not

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 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...

But it's good that the font weightwidth can be selected using the "font weight" parameter, and not using the "gamma"?

@NiLuJe
Copy link
Contributor

NiLuJe commented Mar 28, 2021

Just add a setting to prefer <insert favorite weight here> instead of 400 as the "default" style?

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?

@poire-z
Copy link
Contributor

poire-z commented Mar 28, 2021

But it's good that the font weightwidth can be selected using the "font weight" parameter, and not using the "gamma"?

I guess it is good. It would allow stronger weighting than Gamma can - but unlike Gamma, it needs a re-rendering.
But it might do some changes to people who are used to their fonts the way it is (like making my Bitter Medium synthetically thinner, and they'll have to play with this setting quite often when they change fonts - while before, they got what they got and might have been happy with it).

May be it could have some setting/threshold to not go synthetizing when the requested weight is >50 from the real one we have. I could set it to 150 or 200 to still get my 500 Medium used when this setting is at 400, dunno.

I guess I'll need to test it (so, some bit of work porting it to KOReader...) to see how nice or bothering it gets :)

Just add a setting to prefer instead of 400 as the "default" style?

I guess that if we pick this, we'll have to make our Font Weight toggle a progressbutton like the Contrast one:
image

I've never used Font weight bold (700, vs regular 400), which makes things too bold and ugly. I may use/try 500 if it were available.
But this could get a bit confusing/hard to grasp for users, dunno.

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.

Well, not that easy for us if we go with that progress button bar :/
And it would also apply to each font among embedded fonts in a document with such fonts - and to fallback fonts. Dunno if this would be noticable, or if it would be smooth and unnoticable and just works :)

@NiLuJe
Copy link
Contributor

NiLuJe commented Mar 28, 2021

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?

@poire-z
Copy link
Contributor

poire-z commented Mar 28, 2021

User gets more choices, but that's kind of the point and it's per-font, everybody's happy?

It wouldn't be per-font - but per-document, like the font size, font kerning.
So, when you switch between Bookerly Regular and Bitter Medium (which both would be the same felt weight), you'd need to change that new Font Weight to get them to stay what they naturally are: Bookerly Regular (400) and Bitter Medium (500).

@NiLuJe
Copy link
Contributor

NiLuJe commented Mar 28, 2021

@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 ;).

@NiLuJe
Copy link
Contributor

NiLuJe commented Mar 28, 2021

It wouldn't be per-font - but per-document, like the font size, font kerning.

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 ;).

@NiLuJe
Copy link
Contributor

NiLuJe commented Mar 28, 2021

TL;DR: I think this is great, and I'm much less worried about the frontend integration than @poire-z ;p.

@poire-z
Copy link
Contributor

poire-z commented Mar 28, 2021

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?

Was also wondering if there should be stuff tweaked elsewhere, like if "font-weight: bold" should add 300 instead of setting 700 of if the root node should get the font weight set there.
But no: everything works as normal=400 bold=700, it's just when we ask for a font instance of such weight that this delta weight (from 400 regular) gets added/substracted:

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 ?

if (style->font_weight>=css_fw_100 && style->font_weight<=css_fw_900)
fw = ((style->font_weight - css_fw_100)+1) * 100;
else
fw = 400;
fw += rend_font_embolden;
if ( fw>900 )
fw = 900;
// printf("cssd_font_family: %d %s", style->font_family, style->font_name.c_str());
LVFontRef fnt = fontMan->GetFont(
sz,
fw,
style->font_style==css_fs_italic,
style->font_family,

All the DOM / CSS stuff doesn't need to know the fonts will be tweaked:

switch( pstyle->font_weight )
{
case css_fw_inherit:
pstyle->font_weight = parent_style->font_weight;
break;
case css_fw_normal:
pstyle->font_weight = css_fw_400;
break;
case css_fw_bold:
pstyle->font_weight = css_fw_600;
break;
case css_fw_bolder:
pstyle->font_weight = parent_style->font_weight;
if (pstyle->font_weight < css_fw_800)
{
pstyle->font_weight = (css_font_weight_t)((int)pstyle->font_weight + 2);
}
break;
case css_fw_lighter:
pstyle->font_weight = parent_style->font_weight;
if (pstyle->font_weight > css_fw_200)
{
pstyle->font_weight = (css_font_weight_t)((int)pstyle->font_weight - 2);
}
break;
case css_fw_100:
case css_fw_200:

I'm much less worried about the frontend integration

I'm not that worried about it, I know we could have some fun finding a UI solution for that.
But I'm still worried about breaking existing users experience and fonts habbits.

@virxkane virxkane marked this pull request as ready for review March 29, 2021 04:15
Comment on lines -511 to -514
// 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.
Copy link
Contributor

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... ?

Copy link
Collaborator Author

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.

crengine/src/private/lvfreetypeface.cpp Show resolved Hide resolved
crengine/src/private/lvfreetypeface.cpp Outdated Show resolved Hide resolved
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)
Copy link
Contributor

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.

Copy link
Collaborator Author

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?

crengine/src/private/lvfreetypeface.cpp Outdated Show resolved Hide resolved
@poire-z
Copy link
Contributor

poire-z commented Mar 29, 2021

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.
To test that with current CR or KR, just move away the Bold+BoldItalic variant of your favorite fonts, and you'll get this synthetized bold with no advance change.

@virxkane
Copy link
Collaborator Author

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.

Yes. Using Noto Sans Arabic UI. Of course all other weights variants removed to produce synthetic bold/black. Also all fallback fonts is disabled.
Regular (400):
NotoSans Arabic UI Regular
Black (900) (synthetic)
NotoSans Arabic UI Black(synth)
Black (900) (real)
NotoSans Arabic UI Black(real)

(I still haven't tested this PR :)

So what's stopping you?

About the whole things with advances/RSB, I stick to some comments I wrote (and that you removed :):

I deleted what seemed to me no longer relevant: words in fake bold will be bolder, but not larger than the same word in the regular font. rsb refers to this?

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).

I don’t want to give it up yet.

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.

Well, this is your personal opinion, but in general it is wrong.

@poire-z
Copy link
Contributor

poire-z commented Mar 29, 2021

So what's stopping you?

Come on... I'm just repeating that I'm commenting without seeing the results, so a bit in the black, and that my comments are just suppositions.
(What's stopping me is work - I do crengine bits beween real life work stuff - time, commenting here, discussing with you :) I'm not yet done merging your commits, and there's our base glue code to adapt - my emulator currently crashes - and frontend stuff to do to even get a chance to see what this looks like).

I deleted what seemed to me no longer relevant: words in fake bold will be bolder, but not larger than the same word in the regular font. rsb refers to this?

No, RSB is mostly just related to text highlighting and may be page margins/hanging punctuation (and may be some other stuff I can't think of).
It's all just related to Harfbuzz and its fine positionning: HB does it looking at the font metrics/rules, and gives us some corrections/offsets we use to get super nice positionning.
When synthetizing stuff, making the glyphs bolder and larger, these offsets are then wrong. You can try correcting them by centering, putting some correction on both sides, scaling these offsets depending on how you scaled the advance... and HB may do one unicode char > multiple glyphs or multiple unicode chars > one single glyphs - so there will always be cases where your corrections won't work.
By not touching advances and not horizontally scaling, we get the x positionning to stay consistent.

I can't comment about the Arabic result as I don't read it - but sometimes watching a bit the discussion on github/harfbuzz, people seem sensitive to fine positionning, like the top dot here: real image vs fake image (and see how the cursive is a bit broken at the bottom).
I think we latin/cyrillic people might better appreciate this with hebrew:
image
it's more geometric, and you expect the dot to be centered inside the last glyph there, and the 3 dots below it to also be centered.

I might be fine with having all that a bit distorted if people decide to go with changing the default of 400 - but given that my preferred font is Medum, I'll have to use 500 - and get all 400 fallback fonts synthetized.
And also, what's the use if when using it, my french é à ô may get unbalanced (depending if HB decides to use multiple glyphs, which I have seen it do with some fallback fonts) ?

Well, this is your personal opinion, but in general it is wrong.

Indeed, but broken cursive, mispositionned diacritics, tweaked kerning... are also wrong. The question is which wrong is the most bearable :)

@virxkane
Copy link
Collaborator Author

Come on... I'm just repeating that I'm commenting without seeing the results, so a bit in the black, and that my comments are just suppositions.
(What's stopping me is work - I do crengine bits beween real life work stuff - time, commenting here, discussing with you :) I'm not yet done merging your commits, and there's our base glue code to adapt ................ ).

Yes, of course, me too :)
I meant that you can test this PR in the CoolReader itself.

Indeed, but broken cursive, mispositionned diacritics, tweaked kerning... are also wrong. The question is which wrong is the most bearable :)

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.

@poire-z
Copy link
Contributor

poire-z commented Mar 29, 2021

I meant that you can test this PR in the CoolReader itself.

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 :)
I need to see how pleasant/helpful this can get, before having to think about how to solve the/my other issues.

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.

Well, I think we already have it: Harfbuzz vs non-Harfbuzz :)
Have Harfbuzz be the best/safest/sanest without risks (so, no scaling).
And let FreeType - and may be Harfbuzz light - do scaling, so latin & cyrillic users can use that if they want scaling - while not risking anything for cursive/RTL/diacriticfull languages. That's what currently happens with fake bold.

@virxkane virxkane marked this pull request as draft March 30, 2021 04:41
@virxkane
Copy link
Collaborator Author

virxkane commented Mar 30, 2021

It's all just related to Harfbuzz and its fine positionning: HB does it looking at the font metrics/rules, and gives us some corrections/offsets we use to get super nice positionning.
When synthetizing stuff, making the glyphs bolder and larger, these offsets are then wrong. You can try correcting them by centering, putting some correction on both sides, scaling these offsets depending on how you scaled the advance... and HB may do one unicode char > multiple glyphs or multiple unicode chars > one single glyphs - so there will always be cases where your corrections won't work.
By not touching advances and not horizontally scaling, we get the x positionning to stay consistent.

But you we are already indirectly touching glyph advance:

cur_width += letter_spacing; // only between clusters/graphemes


So if we move the addition of the _synth_weight_advance_add for synthetic weight here, it doesn't get any worse, right?

@poire-z
Copy link
Contributor

poire-z commented Mar 30, 2021

Yes, that's probably the best place to add it if we really want it.
(I think, may be not here but elsewhere, I make sure to not have letter_spacing added for cursive scripts.)

@virxkane
Copy link
Collaborator Author

I think, may be not here but elsewhere, I make sure to not have letter_spacing added for cursive scripts.

Yes i saw it:

if ( letter_spacing > 0 ) {
// Don't apply letter-spacing if the script is cursive
hb_script_t script = hb_buffer_get_script(_hb_buffer);
if ( isHBScriptCursive(script) )
letter_spacing = 0;
}

Yes, that's probably the best place to add it if we really want it.

If nothing breaks, why not?

@poire-z
Copy link
Contributor

poire-z commented Mar 30, 2021

If nothing breaks, why not?

Well, because I'm not wise/knowledgable enough to be sure that "nothing breaks" :)
I get the feeling a HB "cluster" is indeed a unit (a latin/cyrillic letter, a hebrew letter with all its diacritics) to which we can add letter spacing around - I took the risk of this for letter_spacing because it's super rare.
But I'm not really sure it's the case in all scripts/languages, and that we can just scale only every cluster and it will be just nice or bearable.
For me, Harfbuzz does magic, in ways I don't fully understand (and don't need/want to understand :) It looks like it was still doing it with synthetic bold and not scaling. And synthetic bold was previously done only for fallback fonts (like our FreeSerif) that do not come with a bold variants - so the choice between not having bold vs having ok-looking bold was obvious - and we can live with the slight not nice thing.

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 regular|bold toggle do a +100, so actually cycling 100/200/300/400/500/600/700/800/900 ... not really ideal to compare how things look :)
A few thoughts, in random order:

  • it's nice to have, and more useful that the old regular|bold toggle - it makes a thin Palatino Linotype looks quite ok when 400>500.
  • it's also nice to have for people who really love a font that is available in all 9 classic weights - they could now see and enjoy each weight without anything synthetic
  • on eInk, strangely, it makes it bolder, but it still feels gray - I still need to keep applying contrast/gamma to have a feeling of real solid black
  • even if it looks nice/bolder/blacky at first sight, it feels a bit off - mostly because with synthetic bold/italic, we disable hinting, so the little bugs that hinting usually correct (some small correction around the y-axis for letters like r e n t i are no longer done, and quite noticable to me) - and I also can notice some changes in thickness of the vertical strokes. I could ignore that for the benefit of bold, but well, it jumps at my eyes even if I don't want to :) I guess some people can live with it - and it may affect serif fonts more easily than sans-serif fonts. It may also be more noticable on eInk (because a quick test with disabling hinting on my Android phone didn't bother me).
  • when you cycle from 400/500/600 (synthetic bold) and then to 700 where you meet the real bold font), width of words decrease
  • it's indeed quite bothering for my use case of Bitter Medium (500) and Bitter Bold (700). With the setting set to 400, my Medium is syntheticaly made Regular and obviously not what I want. With the setting set to 500, my Medium is there - but then, I get a synthetic Bold (500+300 = 800) :/
  • I have an issue with some fonts at synthetic weights with italic being strange (thinner than expected, looking a bit off the baseline), possibly something with fake italic + fake weight and one overriding the other. Might be me doing bad when porting your code to our single lvfntman.cpp :) No time to look at it currently.
  • as suspected, the dot inside the hebrew letter was navigating outside its centered and eventually crossed its frame, as weight increased :) possibly fixed with your latest commits and doing it per-cluster only

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).
I have mixed feelings, because it's obviously nice to have - but it make a situation where everything worked ok, no matter the combinations (my Bitter mixed weights, and I'm sure some other people might do like I do) as font weights was somehow cap'ed to real font real weights. With this, we get synthetic stuff too easily - and I would want it, for some thin fonts - so a global toggle might not be the solution :/

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.

@virxkane virxkane marked this pull request as ready for review March 30, 2021 12:00
@virxkane
Copy link
Collaborator Author

virxkane commented Mar 30, 2021

@poire-z Ok, thank you.
I update commit The software one-position embolding of the font in the absence of … - changed character advance compensation for synthetic weight.

@virxkane
Copy link
Collaborator Author

Well, because I'm not wise/knowledgable enough to be sure that "nothing breaks" :)

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? :)

@poire-z
Copy link
Contributor

poire-z commented Mar 30, 2021

But if a publisher uses letter-spacing:, it somehow gives us permission to break fine positionning :) as with letter spacing, some text will no longer look natural and benefit from HB magic.

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.
(I need time to test without scaling - to see if the weighting+condensing is still interesting vs. the scaling we get currently - if it is not, I'll follow you - if it is still interesting, I may prefer to stay conservative, dunno yet :)

@Frenzie
Copy link

Frenzie commented Apr 4, 2021

Will give it a go tomorrow.

@NiLuJe
Copy link
Contributor

NiLuJe commented Apr 4, 2021

Ditto ;).

@poire-z
Copy link
Contributor

poire-z commented Apr 4, 2021

@Frenzie
Copy link

Frenzie commented Apr 5, 2021

Opinions on the quality of the result with your various fonts, with/without hinting, the usefulness of the negative and large values, the need for more granularity in the middle... welcome.

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 on the right like for DPI, or maybe even decrease/increase/ like font size.

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.

@poire-z
Copy link
Contributor

poire-z commented Apr 5, 2021

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...):
http://madig.github.io/freetype-web/documentation/tutorial/#d-glyph-transformations
https://www.freetype.org/freetype2/docs/glyphs/glyphs-3.html#section-4
https://www.freetype.org/freetype2/docs/hinting/text-rendering-general.html

And regarding my chronology:
koreader/crengine#309 In september 2019, I added emboldening koreader/crengine@def9bfcb5 - and disabled hinting.
koreader/crengine#317 koreader/koreader#5553 (comment) In november 2019, we switched to FT_LOAD_TARGET_LIGHT .
So, may be the hinting bugs of september were mostly noticable on the x-axis, and shouldn't have been visible after november :)

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;

@buggins buggins merged commit 948820a into buggins:master Apr 5, 2021
@poire-z
Copy link
Contributor

poire-z commented Apr 5, 2021

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.

@virxkane
Copy link
Collaborator Author

virxkane commented Apr 5, 2021

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 ...

@poire-z
Copy link
Contributor

poire-z commented Apr 5, 2021

It's another issue. The commit you mentionned fixed wrongly using synthetic 500 or 600 to make 550.
But when using 550 (that you only allowed recently), which is mid distance from 400 and 700, both real 400 and 700 might get the same match/score and are both candidates. Depending on stuff, one of these might be instantiated, and will be the one that will win.
It's possible a +1 or -1 somewhere in the weight scoring might help privileging one or the other (I'd prefer privileging 400 in this case, so we get 400 425 450 475 500 525 550 possibly build from the regular, that you 4pda user seems to prefer too).

@virxkane
Copy link
Collaborator Author

virxkane commented Apr 5, 2021

It's another issue. The commit you mentionned fixed wrongly using synthetic 500 or 600 to make 550.
But when using 550 (that you only allowed recently), which is mid distance from 400 and 700, both real 400 and 700 might get the same match/score and are both candidates. Depending on stuff, one of these might be instantiated, and will be the one that will win.
It's possible a +1 or -1 somewhere in the weight scoring might help privileging one or the other (I'd prefer privileging 400 in this case, so we get 400 425 450 475 500 525 550 possibly build from the regular, that you 4pda user seems to prefer too).

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?
In my opinion, 550 should be removed from the list of options.

@poire-z
Copy link
Contributor

poire-z commented Apr 5, 2021

You have the same situation with synthetic 500 if the user has real 400 and a 600 fonts.
I understand your point (that you shared on your forum) - and I share it - when real 400 and 700, that 600 should be made from the 700: the less transformation to a real font, the better.
But when they are equal scores, it's no longer a font designer choice: it's our choice :)
(Given that your user and I feel like privileging the regular, I think our choice could be this - haven't yet looked at the code how this could be done.)

@poire-z
Copy link
Contributor

poire-z commented Apr 5, 2021

@virxkane : I'm not really familiar with how CoolReader handles its settings.
But it seems your:
static int int_option_weight[] = { 100, 200, 300, 400, 425, 450, 475, 500, 525, 550, 600, 650, 700, 800, 900, 950 };
does not limit the value frontends can provide. I can provide any value, and this will use it:

        } else if (name == PROP_FONT_BASE_WEIGHT) {
            int v = props->getIntDef(PROP_FONT_BASE_WEIGHT, 400);
            if ( LVRendGetBaseFontWeight() != v ) {
                LVRendSetBaseFontWeight(v);

which is fine for me. Am I right?
If yes, just out of curiosity, in which contexts is static int int_option_weight used?

@virxkane
Copy link
Collaborator Author

virxkane commented Apr 5, 2021

But it seems your:
static int int_option_weight[] = { 100, 200, 300, 400, 425, 450, 475, 500, 525, 550, 600, 650, 700, 800, 900, 950 };
does not limit the value frontends can provide.

It's to validate settings at program startup.

@virxkane virxkane deleted the font-weight branch April 5, 2021 12:32
@virxkane
Copy link
Collaborator Author

virxkane commented Apr 5, 2021

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.
But I don’t have time for this yet and I’m tired enough of it PR.
You will have to solve this problem yourself, just let me know what the final solution is.

@poire-z
Copy link
Contributor

poire-z commented Apr 5, 2021

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 :)

@ptrm
Copy link

ptrm commented Apr 5, 2021

#274 (comment) updated files, including latest commit 32da167:
kobo-frontend+libscre_coolreader274_20210405.zip
crengine+frontend_diffs_for_coolreader274_20210405.zip

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:

  1. Am I correct to notice that native hinting is respected at non-zero values? Arched letters seem to get a bit higher as expected with hinting toggling, and I saw some discussion above that hinting may need to be dropped for non-zero weight delta.
  2. Is +0.5 possible? My preferred / usable set would be indeed [-1, 0, +0.5, +1] (I never really used the bold font toggle).
  3. Am I correct to assume that I can finally put both regular and medium (500) weights into the font dir in which case there won't be any procedural bolding on +1, but just the medium weight be picked up (and regular for 0, instead of currently being overriden by medium weights?)

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!

@virxkane
Copy link
Collaborator Author

virxkane commented Apr 5, 2021

(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 :)

Didn't know it worked like that. Well it's too late now.
But this is definitely not the ever worst email :)

@poire-z
Copy link
Contributor

poire-z commented Apr 5, 2021

(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 ;) )

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" :)
Anyway, the comfort the fatness can bring counteracts the possible loss in quality.

Am I correct to notice that native hinting is respected at non-zero values? Arched letters seem to get a bit higher as expected with hinting toggling, and I saw some discussion above that hinting may need to be dropped for non-zero weight delta.

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?

Is +0.5 possible? My preferred / usable set would be indeed [-1, 0, +0.5, +1] (I never really used the bold font toggle).

Yes, it will be - with a fine tuning dialog allowing 0.25 steps.

Am I correct to assume that I can finally put both regular and medium (500) weights into the font dir in which case there won't be any procedural bolding on +1, but just the medium weight be picked up (and regular for 0, instead of currently being overriden by medium weights?)

You are correct.
And regular will be used to synthetized 0.25 (=425), medium to synthetize 0.75 (=475) - and currently randomly one of them to synthetize 0.50 (=450) :) (that will be fixed).
I added something (for KOReader): if you have only the medium (500), I correct it on load so it's tagged as being 400 - so we can keep having it at 0 (=400) instead of having a synthetic 400 made from the 500. It's better when comparing fonts to always have them being "real" when the setting is "0".

Thanks for the good feedback.

@virxkane
Copy link
Collaborator Author

virxkane commented Apr 5, 2021

Sorry gentlemens, but maybe here we will discuss how it looks in CoolReader? :)
https://github.com/virxkane/coolreader/releases/tag/v3.2.55-20210404-2

@NiLuJe
Copy link
Contributor

NiLuJe commented Apr 5, 2021

Yep, not my use-case either, but it's a fantastic addition, kudos @virxkane and everyone involved in the testing ;).

@yparitcher
Copy link

@poire-z
was busy the last few days, your diff errors on the latest KOReader master so i couldn't test :(

@poire-z
Copy link
Contributor

poire-z commented Apr 11, 2021

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.
But I don’t have time for this yet and I’m tired enough of it PR.
You will have to solve this problem yourself, just let me know what the final solution is.

Solution I've found at koreader/crengine@207fc79

@virxkane
Copy link
Collaborator Author

Solution I've found at koreader/crengine@207fc79

OK, thanks you.

@virxkane
Copy link
Collaborator Author

virxkane commented Apr 15, 2021

OK, experiment using 26.6 units is considered unsuccessful.
PR #278

@virxkane
Copy link
Collaborator Author

virxkane commented Sep 3, 2021

Yep, not my use-case either, but it's a fantastic addition, kudos @virxkane and everyone involved in the testing ;).

I don't see myself here https://github.com/koreader/koreader/releases/tag/v2021.05
:)

@Frenzie
Copy link

Frenzie commented Sep 3, 2021

My bad, added. ;-)

@virxkane
Copy link
Collaborator Author

virxkane commented Sep 3, 2021

@Frenzie Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants