bump crengine: round FT metrics, split text drawing by script#5628
bump crengine: round FT metrics, split text drawing by script#5628poire-z merged 3 commits intokoreader:masterfrom
Conversation
Includes: - Fonts: round FT metrics instead of floor'ing them - Fonts: switch to no hinting when native hinting fails - Fonts: fix issue with Harfbuzz fallback font drawing - Text: split measuring and word drawing by unicode script - Page splitting: fix small memory leak - Fix "background-color: black" ignored on inline elements - Fix decoding of recent MOBI files - Hardcoded elements list: add <font> base/xtext.cpp: small cleanup, no logic change
|
Pinging font aesthetes @Frenzie @NiLuJe . Unit tests are failing because toc, nb pages changed with the font metric rounding instead of floor. I didn't expect such a change (+10% more pages - about visual changes, I'm asking you) on the emulator. Before, 297 pages with font metric floor'ed (>>6): After 325 pages with font metrics round'ed: That's with the default setting Kerning best (harfbuzz) and Hinting auto. |
|
We've found the reason behind the "too close together" feeling we had about "best", haven't we? ;). And, yeah, I definitely prefer the second version on low-dpi (i.e., here ;p). And, yeah, should be slightly less noticeable on high-dpi devices. |
|
The second is better because the first is too tightly packed. (However, in this case I like the overall visual result better in the first picture due to how the line breaks coincidentally turned out.) On such a low resolution there's just no way around the fact that it's either too thin or too wide. For legibility, too wide is arguably better, and rounding is more accurate regardless. |
|
Yeah, if I had something to complain about, that'd indeed be related to word spacing in the second example. But we have a button for that, IIRC ;). |
|
OK, thanks :) So, good to go fix unit test! |
|
Random stupid comment: you mentioned it needing a cache rebuild, is that currently enforced with a cache version bump or whatever it is you usually do for that kind of stuff? ^^. |
|
Yes, I did not forget to bump it this time :) |
|
Oh, okay, yeah, that makes sense ;). |
|
Follow up to @ptrm https://gitter.im/koreader/koreader?at=5dd92577df948d0f588596de
It would feel awkward to implement tunable buggy behaviour :) crengine already supports With a style tweak like (but there's no granularity below 1 screen px.)
Yes, the space gets also its width rounded. koreader/frontend/ui/data/creoptions.lua Line 421 in 4740ab1 Does that changes things for you, if you use the min value? It's a bit complex, and I'm not really familiar with that code: I get that it uses that word gap setting (50%, 75%, 100%) to set the minimal value for the width of a space: so a space can get shrinked to 75% of its original width (floor'ed or rounded, shouldn't really matter as it's reduced to 75%, so less than the original width. Not sure how this floor/round change affects all that. There is also this limitation to the word gap min value: |
I would love it. 1px at 300dpi seems a good start to me :)
I do notice, but only in certain lines, those more unaffected by justification I think. Glad to hear there's math that supports it. Below are screenshot crops before and after the PR change, with provisional guides: Generally there now seems to be more difference betwheen standard space width and my
Yes, I treat it as a "squeeze treshold" in justification, and in this respect it helps me keep balance between congestion and very loose spaces between words :) TL;DR: I would much love to see negative letter spacing support, even a pixel will be good (for a start ;) ) EDIT: The font visible on screenshots is Baskervaldx. |
|
And of course, making a bug a feature was not my intention :) I think I only mind the whitespace width, not so much wider-looking words. Which on second thought leads me to conclusion that I might actually want word-spacing support implemented. How does this look in terms of complexity? ;) |
That's what it is :)
I'm still not sure if it's just natural and correct behaviour with longer words because of rounding, or if there's a bug in that line wrapping/text justification code :|
I guess that "word-spacing: xxx" would make sense only when text is left aligned. What about when text is justified, and spacing is variable and adjusted for each line? Btw, on your 2nd screenshots, the spaces looks similarly sized on the 1st (Zdawalo...) and the 2nd (Zadnego). It looks more irregular on your 1st screenshot (with that 2nd line really more condensed than the others). |
I'm aware of the squeezing caused by justification (as in 2nd line), I did not however point out that I meant mosty the first line which as a whole is a single paragraph. The squeezing of spaces is alright with me, and I made it look so narrow with word spacing set to 50 :) So the squeeze part is ok with me, because I can still tune it.
Exactly :) I mean only differences in the spaces unaffected by justification. Should have probably taken a screenshot with text-align: left; ;)
That's Kobo Aura One. I have also Clara HD from 7th hw revision to test on if this matters (and a Forma for some time, packed to be sold ;> ).
I think in case of justified text it would be the reference space width, instead of the whitespace character width defined in font metrics. Here's a quick html example. Clicking the text toggles between text-align: left and justify. The word-spacing is stretched and squished, but remains somehow proportionally smaller or bigger than the other columns. |
|
@poire-z: Doesn't that check look dodgy? Comment says 1/4, yet code does 3/4, and I'm not sure each of 'em actually use the same unit (em or pt vs. px and/or width vs height?). |
|
People say all kinds of things about how wide a space should be. As wide as an i, as wide as an o… Personally I like something closer to half-o (or perhaps rather, r-width or s-width). What is it that code actually does? Does it do something halfway sensible, like ensuring the space won't be condensed below 3/4 of the size of a space as specified by the font or does it actually do something nonsensical like "3/4 of font size" as the comment says? |
|
Another data point: https://en.wikipedia.org/wiki/Thin_space So, I'd naively assume that this is indeed missing an em-to-px conversion somewhere (and, also, the fraction's bogus ;p). |
|
But thin spaces have a very specific purpose. For example, instead of: bla—bla—bla You should write (or with a hair space instead): bla — bla — bla This is a bit too tight: It is, however, the width of an i, so at it does fit at the extreme low end of the range from |
To clarify myself, I did not mean to change any of the defaults, even if they are to be varying between releases. What you wrote, though, is to me one of the reasons to implement word-spacing support. I mean, this would not add any UI elements, but could be used as a css tweak if anyone minds to have a different setting. |
It returns something that can be substracted to the space width (or, and that's equivalent: to the width of the (current word + follow up space included)).
But... it looks like that "Word Gap" setting has no effect when there is no justification involved :) When not justified, the spaces get their normal width (set by the font designer).
I don't think there's another "reference space width" than the currently used font's space's width. It's the font designer choice, his decision on the width of a space. Laying out text with Freetype will just use this space width. I don't think browsers would dare doing differently (except of course if one specifieds word-spacing: CSS property). |
We could. |
It's stupid then. |
That's true. I wish for actual word spacing setting, not the space stretchabilty one. |
I can barely see a difference at 260 DPI. This is my opinion on how justification has always been. |
Yes, and I think this belongs to another issue :) I did not wish to change what's currently the default measure for spaces.
True. Even if I did, it was only a sensation of big disproportion between smallest and biggest space on the screen. |
|
Some other idea, because:
So, that other idea: As we all thought/expected that --- word gap percentage
-DCREREADER_CONFIG_WORD_GAP_SMALL = 50
-DCREREADER_CONFIG_WORD_GAP_MEDIUM = 75
-DCREREADER_CONFIG_WORD_GAP_LARGE = 100
+-- word gap condensing percentage and space width percentage
+DCREREADER_CONFIG_WORD_GAP_SMALL = {50, 80}
+DCREREADER_CONFIG_WORD_GAP_MEDIUM = {75, 90}
+DCREREADER_CONFIG_WORD_GAP_LARGE = {100, 100}Medium would hard-squeeze the regular space width to 90% of its width in the font (or we could use -1, -2, values in pixels, or else) - and this would allow users to combine variations of all that to their liking. Any thought?
Not sure if you mean "I do" or "I don't see a difference" with that "I can barely" :) |
|
@poire-z: Yep, that'd be perfectly fine by me ;). |
+DCREREADER_CONFIG_WORD_GAP_SMALL = {50, 80}I would be delighted by this solution, whatever the unit. (Or maybe |
|
Yeah, I was thinking % makes sense as a unit, as it's more flexible regarding DPI variance, since that's an "always-on" setting, regardless of the default ;). |
Well, I don't think that's possible :) We try to find a wrap point by reducing stuff. If we don't, well, we use the last one found, and the, we have to stretch where it's allowed (the spaces) if we want text justification. If we impose a max stretch, we might not be able to do the full justification. |
Ok :)
Justified and hyphenated |
I mean I can, but only if I'm trying to. I don't find it more or less legible, or consciously notice slightly less text on the screen.
It depends, it's usually fine. But if a few extra pixels make the situation worse it's because the line breaking algorithm has always been mediocre. However, I don't think it makes it worse. It just makes it different, which may very well make it appear worse in situations where it was previously near-perfect. At the same time it'll have magically fixed paragraphs where it was previously terrible. Logically speaking this should pretty much be a 50/50 toss up. |
…er#5628) Includes: - Fonts: round FT metrics instead of floor'ing them - Fonts: switch to no hinting when native hinting fails - Fonts: fix issue with Harfbuzz fallback font drawing - Text: split measuring and word drawing by unicode script - Page splitting: fix small memory leak - Fix "background-color: black" ignored on inline elements - Fix decoding of recent MOBI files - Hardcoded elements list: add <font> base/xtext.cpp: small cleanup, no logic change
|
Spacing change looks distinct enough, worth trying a 10-page read to check :) Is this thought to be (if picked) a togglable feature? |
Probably not (at most something hardcoded as yes/no for some typography languages if that would make sense per language).
You mean you appreciate the reduction of spaces width? |
|
@poire-z: Did you get the Before/After wrong, by chance? (After has wider spaces between words, but narrower inter-glyph in-word spacing on affected lines). In any case, at a quick glance, I'm happy with !"Before" (i.e., ... 5d.png) ;). |
|
And, yeah, if possible, I'd be happy with that being a fancy toggle, too. |
Indeed :/ Fixed.
You mean you like it?! With all the kerning 'n hinting being messed with? I'm a bit surprised :) |
|
@poire-z: Yeah, me too ;p. Worth trying it out for real, though, but the more I see it, the more it grows on me, good job :). |
|
That looks interestingly and surprisingly well! I didn't realize that eyes are actually focusing on one line during reading, so relative spacing is gone in the peripheral vision. |
Well, no :) That was just a quick try to get a feeling if it was worth going on doing it well :)
Should go with that well. |
My main idea was crap :) We can, because we can still adjust the widths of inter word spaces. And it's a lot easier with less edge cases to handle that doing it when making lines (added bonus: no need for re-rendering if we just change that setting, as we're just positionning words on a line, and the line content does not change, so paragraphs heights can't change, and the cached rendering is still valid). Anyway, I don't know if it should be another toggle, or integrated in the Also, not sure about the metric to use/trigger/limit the allowed % of added letter spacing (% of what?). #define PROP_FORMAT_SPACE_WIDTH_SCALE_PERCENT "crengine.style.space.width.scale.percent"
#define PROP_FORMAT_MIN_SPACE_CONDENSING_PERCENT "crengine.style.space.condensing.percent"
// % of unused space on a line to trigger hyphenation, or addition of letter spacing for justification
#define PROP_FORMAT_UNUSED_SPACE_THRESHOLD_PERCENT "crengine.style.unused.space.threshold.percent"
// Max allowed added letter spacing (% of font size)
#define PROP_FORMAT_MAX_ADDED_LETTER_SPACING_PERCENT "crengine.style.max.added.letter.spacing.percent"
[...]
/// set space glyph width scaling percent option (10..500%)
// (scale the normal width of all spaces in all fonts by this percent)
void setSpaceWidthScalePercent(int spaceWidthScalePercent);
/// set space condensing line fitting option (25..100%)
// (applies after spaceWidthScalePercent has been applied)
void setMinSpaceCondensingPercent(int minSpaceCondensingPercent);
/// set unused space threshold percent option (0..20%) default 5%
void setUnusedSpaceThresholdPercent(int unusedSpaceThresholdPercent);
/// set max allowed added letter spacing (0..20% of font size) default 0%
void setMaxAddedLetterSpacingPercent(int maxAddedLetterSpacingPercent);(The unused_space_threshold_percent was hardcoded to 5% - and I guess it will stay at 5 and won't be tweakable from the UI - but @ptrm might want to experiment with it.) Some screenshots, on the emulator with a big font size (otherwise, 5% will give 0px letter spacing) - but that's about what we would get on our higher dpi devices. The following is with using that maxAddedLetterSpacingPercent as a 3rd setting of the word spacing menu: So, none, max 5%, max 15%. Not sure how much this is annoying with ligatures: For those on Kobo willing to check for themselves and decide about the % to propose, and see if the metrics I used make sense (and that would not mind the number in the right margin): --- a/frontend/document/credocument.lua
+++ b/frontend/document/credocument.lua
@@ -810,6 +810,7 @@ function CreDocument:setWordSpacing(values)
self._document:setIntProperty("crengine.style.space.width.scale.percent", values[1])
logger.dbg("CreDocument: set space condensing", values[2])
self._document:setIntProperty("crengine.style.space.condensing.percent", values[2])
+ self._document:setIntProperty("crengine.style.max.added.letter.spacing.percent", values[3] or 0)
endAnd some adaptation (used your prefered value as the common base) in defaults.lua or defaults.persistent.lua: |
|
@poire-z: I'd possibly reverse the settings, as it effectively tightens word spacing (i.e., put ~15 in SMALL and 0 in LARGE). Other than that, it's going to take a bit of time to find settings that I like, but 15% doesn't seem too egregious as a default maximum on the Forma ;). (It's probably a tad high for my tastes & settings, but it's not awful, and makes the feature prominently visible). (FWIW, it goes as high as 6px w/ my settings, which is where things starts to feel a tad too much for me. The lines at 4 px are perfectly fine, on the other hand). |
|
@poire-z: I'm always seeing multiples of two in the margins. Expected, or coincidence? |
Coincidence. Should stay the same numbers for a same font/font size (the % is letterspaciing_in_px/font size_in_px) - so try changing that :) |
|
The issue we might have is that at some font size, because of rounding, we may get stuck to some low values (1px=2%, 2px=4%), or get too easily a larger value (1px=4%, 2px=9), so a trigger at 5% have more steps in the first case, and only one in the 2nd case. Although just writting that, this looks like the expected bahaviour :)
The numbers that you see are %, not px. So, if you see 2 4 6, it's probably 1px, 2px, 3px. |
|
OK, proposed in koreader/crengine#341. |

















Includes koreader/crengine#318
base/xtext.cpp: small cleanup, no logic change
This change is