Skip to content

bump crengine: round FT metrics, split text drawing by script#5628

Merged
poire-z merged 3 commits intokoreader:masterfrom
poire-z:bump_crengine
Nov 22, 2019
Merged

bump crengine: round FT metrics, split text drawing by script#5628
poire-z merged 3 commits intokoreader:masterfrom
poire-z:bump_crengine

Conversation

@poire-z
Copy link
Copy Markdown
Contributor

@poire-z poire-z commented Nov 22, 2019

Includes koreader/crengine#318

  • 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. Closes kobo aura hd Chinese mobi gibberish #5599
  • Hardcoded elements list: add

base/xtext.cpp: small cleanup, no logic change


This change is Reviewable

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
@poire-z
Copy link
Copy Markdown
Contributor Author

poire-z commented Nov 22, 2019

Pinging font aesthetes @Frenzie @NiLuJe .

Unit tests are failing because toc, nb pages changed with the font metric rounding instead of floor.
(It was quite hard to validate that as when you swtich libcrengine.so between one with the font metric rounding vs one without, the cache previously made with the other is reused if you don't manually trash it...)

I didn't expect such a change (+10% more pages - about visual changes, I'm asking you) on the emulator.
It will probably be less noticable on high dpi device.

Before, 297 pages with font metric floor'ed (>>6):
floor

After 325 pages with font metrics round'ed:
round

That's with the default setting Kerning best (harfbuzz) and Hinting auto.
Any feeling of which is better?
(Hoping the 2nd is - or that at least it's OK enough so I can go on updating unit tests...)

@NiLuJe
Copy link
Copy Markdown
Member

NiLuJe commented Nov 22, 2019

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.

@Frenzie
Copy link
Copy Markdown
Member

Frenzie commented Nov 22, 2019

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.

@NiLuJe
Copy link
Copy Markdown
Member

NiLuJe commented Nov 22, 2019

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

@poire-z
Copy link
Copy Markdown
Contributor Author

poire-z commented Nov 22, 2019

OK, thanks :) So, good to go fix unit test!
(I'm also biased to things condensed, so I prefered the 1st).

@poire-z poire-z merged commit 4740ab1 into koreader:master Nov 22, 2019
@poire-z poire-z deleted the bump_crengine branch November 22, 2019 22:54
@NiLuJe
Copy link
Copy Markdown
Member

NiLuJe commented Nov 22, 2019

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

@poire-z
Copy link
Copy Markdown
Contributor Author

poire-z commented Nov 22, 2019

Yes, I did not forget to bump it this time :)
The issue with the cache I had is that I rebuild crengine with only the font rounding commit reverted, keeping the version bump commit, so same in both libcrengine.so builds. And no part of the cache hashes does (nor need to) take into account font rounding vs flooring. So, nothing to invalidate the cache (and no need to).
(The cache stores the rect(x,y,w,h) of each paragraph, and was reused, keeping the same sized rects (and nb of pages). With the font metrics change, I guess paragraph rects were not filled the same way by their text. So, although I haven't looked for that this evening, I think some text could have overflowed their paragraph cached rect (measured with the other libcrengine.so) and were probably written over the next paragraph :)

@NiLuJe
Copy link
Copy Markdown
Member

NiLuJe commented Nov 22, 2019

Oh, okay, yeah, that makes sense ;).

@robert00s robert00s added this to the 2019.12 milestone Nov 23, 2019
@poire-z
Copy link
Copy Markdown
Contributor Author

poire-z commented Nov 23, 2019

Follow up to @ptrm https://gitter.im/koreader/koreader?at=5dd92577df948d0f588596de

Actually I was to ask whether it's on a timeline to implement switchable letter-spacing behaviour (new rounded with the old floored one), or even adjustable letter-spacing like word-spacing already is ;) ? That's not to say closer-spaced letters don't look good, I quite like it (and with 23 Baskervaldx font, after today's update my page count increased from 322 to just 329), just that some fonts have too big spacings

It would feel awkward to implement tunable buggy behaviour :)
The floor'ing indeed made things more condensed (and I quite like things condensed), but it also makes letter spacing irregular. The round'ing make that more regular, and is the way to do that right.
On my Kobo, I don't notice the change. And I guess those who do will get used to it and won't notice it after some time.

crengine already supports letter-spacing, but, for some reason, it limits it to positive values.
A 2mn hack made it work with negative values (should I PR that?)
Original:
Reader_2019-Nov-23_163406

With a style tweak like *{letter-spacing: -1px;}:
Reader_2019-Nov-23_163427

(but there's no granularity below 1 screen px.)
I don't think it deserves a toggle button in the bottom menu.

And the setting we currently have is minimum word spacing if the word is too long to fit in a line
Just assume the rounding applies now to the whitespace char as well
which is also defined in font metrics afair

Yes, the space gets also its width rounded.
But the spacing handling with justifications is a bit more complex.
I don't really know why you all notice some change in word spacing (I don't) with that floor>round change.
Help text (hold on it) for the Word Gap toggle:

help_text = _([[Tells the rendering engine how much each 'space' character in the text can be reduced from its regular width to make words fit on a line (100% means no reduction).]]),

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:
https://github.com/koreader/crengine/blob/cc9af0ad913dc1357bd968dd503096cbb1fbbdbc/crengine/src/lvtextfm.cpp#L1590-L1591
https://github.com/koreader/crengine/blob/cc9af0ad913dc1357bd968dd503096cbb1fbbdbc/crengine/src/lvtextfm.cpp#L2311-L2316
https://github.com/koreader/crengine/blob/cc9af0ad913dc1357bd968dd503096cbb1fbbdbc/crengine/src/lvtextfm.cpp#L2487-L2497

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.
So, a lower value will get more chances to put more words on the line. But on some lines, this won't help (if the last word is too long or not breakable at a good point), so we'll break earlier, and then we'll have more space to fill. So, that width to fill will be distributed to all spaces, and so these spaces may end up being larger than their original width.

Not sure how this floor/round change affects all that.

There is also this limitation to the word gap min value:
https://github.com/koreader/crengine/blob/cc9af0ad913dc1357bd968dd503096cbb1fbbdbc/crengine/src/lvtextfm.cpp#L2495-L2497
Dunno, may be there's some bug in that part, and/or the space width being rounded makes it larger, and limit its minimal value to a point where it's noticable.
Or the rounding end up making all words larger, and there's less opportunity to break, or less spaces on a line to distribute the blank, so the fewer spaces gets larger. Or something else, dunno.

@ptrm
Copy link
Copy Markdown
Contributor

ptrm commented Nov 23, 2019

@poire-z crengine already supports letter-spacing, but, for some reason, it limits it to positive values.
A 2mn hack made it work with negative values (should I PR that?)

I would love it. 1px at 300dpi seems a good start to me :)

Yes, the space gets also its width rounded.
But the spacing handling with justifications is a bit more complex.
I don't really know why you all notice some change in word spacing (I don't) with that floor>round change.

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:
20191123_174256
20191123_174232

Generally there now seems to be more difference betwheen standard space width and my50 word gap setting.

Does that changes things for you, if you use the min value?

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.

@ptrm
Copy link
Copy Markdown
Contributor

ptrm commented Nov 23, 2019

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

@poire-z
Copy link
Copy Markdown
Contributor Author

poire-z commented Nov 23, 2019

Yes, I treat it as a "squeeze treshold" in justification

That's what it is :)
On your screenshots, I guess on that 2nd line ("zadnego..."):

  • before, spaces were very squeezed, and it made room for "lata" (because smaller words allowed with the minimal space width to make it fit).
  • after, each word being a bit larger (whether the space itself is larger too or not), there's less room to fit "lata", even when squeezing the spaces, so "lata" ends up on the next line, making all the spaces larger.
    I'm not sure you'd actually like it to have the spaces even more reduced so to fit "lata" - as the words are themselves larger, the spaces may become too tiny. So, might be letter-spacing: -1px will please you. (What's your device? If it's a kobo, I can give you a libcrengine.so with negative letterspacing allowed - to see if it's "the"/"your" solution :)

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

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

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?
(Or, dunno, might be word-spacing could be handled just as letter spacing, but applied only to spaces?!)

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

@ptrm
Copy link
Copy Markdown
Contributor

ptrm commented Nov 23, 2019

@poire-z On your screenshots, I guess on that 2nd line ("zadnego...")
[...]
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.

I'm not sure you'd actually like it to have the spaces even more reduced so to fit "lata" - as the words are themselves larger, the spaces may become too tiny

Exactly :) I mean only differences in the spaces unaffected by justification. Should have probably taken a screenshot with text-align: left; ;)

What's your device? If it's a kobo, I can give you a libcrengine.so with negative letterspacing allowed

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 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?
(Or, dunno, might be word-spacing could be handled just as letter spacing, but applied only to spaces?!)

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.

@NiLuJe
Copy link
Copy Markdown
Member

NiLuJe commented Nov 23, 2019

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

@Frenzie
Copy link
Copy Markdown
Member

Frenzie commented Nov 23, 2019

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?

@NiLuJe
Copy link
Copy Markdown
Member

NiLuJe commented Nov 23, 2019

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

@Frenzie
Copy link
Copy Markdown
Member

Frenzie commented Nov 23, 2019

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

It is, however, the width of an i, so at it does fit at the extreme low end of the range from i to o.

@ptrm
Copy link
Copy Markdown
Contributor

ptrm commented Nov 23, 2019

@Frenzie People say all kinds of things about how wide a space should be.

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.

@Frenzie
Copy link
Copy Markdown
Member

Frenzie commented Nov 23, 2019

No need to implement anything (although standard CSS properties are useful); just remove some artificial restriction that doesn't allow space condensing smaller than whatever this is (that's at "0"):

Screenshot_2019-11-23_19-57-13

@poire-z
Copy link
Copy Markdown
Contributor Author

poire-z commented Nov 23, 2019

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

What is it that code actually does?

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)).
So, with word gap medium default of 75%, it returns 25% of the space width (which, when substracted, will give 75% of the space width).
It looks like it also ensures it does not return more than 3/4*font_size (so, 0.75em).
So, the comment typographic rule: don't use spaces narrower than 1/4 of font size is accurate with what it does (whether it's a typographic rule or not, dunno).
So, it actually limits the minimal space width (so, me trying with DCREREADER_CONFIG_WORD_GAP_SMALL = 20 or 30 did not work :)

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

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 think in case of justified text it would be the reference space width, instead of the whitespace character width defined in font metrics

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

@poire-z
Copy link
Copy Markdown
Contributor Author

poire-z commented Nov 23, 2019

just remove some artificial restriction that doesn't allow space condensing smaller than whatever this is

We could.
But @ptrm wish is for smaller spaces on non-justified lines, where that setting won't apply.

@Frenzie
Copy link
Copy Markdown
Member

Frenzie commented Nov 23, 2019

So, it actually limits the minimal space width

It's stupid then.

@ptrm
Copy link
Copy Markdown
Contributor

ptrm commented Nov 23, 2019

@Frenzie just remove some artificial restriction that doesn't allow space condensing smaller than whatever this is

@poire-z We could.
But @ptrm wish is for smaller spaces on non-justified lines, where that setting won't apply.

That's true. I wish for actual word spacing setting, not the space stretchabilty one.

@Frenzie
Copy link
Copy Markdown
Member

Frenzie commented Nov 23, 2019

I don't follow what you guys are referring to. Changing the space condensing (word gap) setting completely changes the layout of the left-aligned text:

Screenshot_2019-11-23_20-03-30

@Frenzie
Copy link
Copy Markdown
Member

Frenzie commented Nov 23, 2019

And we all mostly did not

I can barely see a difference at 260 DPI. This is my opinion on how justification has always been.

@ptrm
Copy link
Copy Markdown
Contributor

ptrm commented Nov 23, 2019

@poire-z Adding that word-spacing support (to reduce the width of regular space in all contexts) might actually be the solution to @ptrm original wish, and getting less spacing in justified text after thar round>floor change (Dunno how much work that would be thus.)

Yes, and I think this belongs to another issue :) I did not wish to change what's currently the default measure for spaces.

I'm a bit relunctant touching anything of that, as I don't see any issue :)
And we all mostly did not, until that floor>round change showed up more spacing - which might not be directly related to that condensing thingy.

True. Even if I did, it was only a sensation of big disproportion between smallest and biggest space on the screen.

@poire-z
Copy link
Copy Markdown
Contributor Author

poire-z commented Nov 23, 2019

Some other idea, because:

  1. handling negative CSS letter-spacing is not fun: we don't want to go backward and have negative widths, so needs quite a few checks in many places, and I don't know how that would do with Harfbuzz.
  2. handling CSS word-spacing, even if it would just be implemented as reducing the width of spaces, would waste a CSS slot (we're at 58 on a max of 64 :)
  3. these CSS are so not common, and even, I'd rather not like the publisher to mess with regular font settings - and I don't care much for them, so I'm fine if we keep not supporting them.
  4. fixing our space issues or wishes with a bunch of * {letter-spacing: -1px ; word-spacing: -0.12em} styletweaks feels tedious and ugly.

So, that other idea:

As we all thought/expected that Word Gap: small | medium| large would influence the regular spacing (and not only condensation when finding line wrap candidates), may be use it to propagate to crengine a second internal setting.
We would then have

--- 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.
And it would give that toggle a more explicite visual effect on the text, while conforming to what it's name announces.

Any thought?

I can barely see a difference at 260 DPI. This is my opinion on how justification has always been.

Not sure if you mean "I do" or "I don't see a difference" with that "I can barely" :)
Not sure either about your opinion of how justification has always been, but I assume you think it's not marvellous. Some discussion about it at koreader/crengine#307 (comment) and followups.

@NiLuJe
Copy link
Copy Markdown
Member

NiLuJe commented Nov 23, 2019

@poire-z: Yep, that'd be perfectly fine by me ;).

@ptrm
Copy link
Copy Markdown
Contributor

ptrm commented Nov 23, 2019

+DCREREADER_CONFIG_WORD_GAP_SMALL = {50, 80}

I would be delighted by this solution, whatever the unit. (Or maybe {50, 80, 105} for max space stretch, if that won't complicate breaking words or anything :3 )

@NiLuJe
Copy link
Copy Markdown
Member

NiLuJe commented Nov 23, 2019

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

@poire-z
Copy link
Copy Markdown
Contributor Author

poire-z commented Nov 23, 2019

max space stretch

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.
(Do you use text justification, or have text left aligned?)

@ptrm
Copy link
Copy Markdown
Contributor

ptrm commented Nov 23, 2019

Well, I don't think that's possible :)

Ok :)

(Do you use text justification, or have text left aligned?)

Justified and hyphenated

@Frenzie
Copy link
Copy Markdown
Member

Frenzie commented Nov 24, 2019

Not sure if you mean "I do" or "I don't see a difference" with that "I can barely" :)

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.

Not sure either about your opinion of how justification has always been, but I assume you think it's not marvellous.

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.

mwoz123 pushed a commit to mwoz123/koreader that referenced this pull request Mar 29, 2020
…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
@poire-z
Copy link
Copy Markdown
Contributor Author

poire-z commented Apr 27, 2020

Just had a quick try at increasing letter spacing when the spacing between words would get too wide (which I think/read that it is a solution used in paper books).
Sharing some screenshots, just to get your (at least you who contributed to that previous discussion) opinion if it is an acceptable solution, or if you have a strong feeling it should not be (if you like your words the way they are, as we'd be messing with kerning and Harfbuzz fine positionning :)

(click, click, Ctrl-Tab for comparing)
Before:
After

After:
Before

That's just a buggy quick first try: some end of lines overflows, some candidate lines are not handled, but there's 4 lines that got processed with 1px added to each letter.

@ptrm
Copy link
Copy Markdown
Contributor

ptrm commented Apr 27, 2020

Spacing change looks distinct enough, worth trying a 10-page read to check :)

Is this thought to be (if picked) a togglable feature?

@poire-z
Copy link
Copy Markdown
Contributor Author

poire-z commented Apr 27, 2020

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).
Of if it should, may be a third parameter added to: DCREREADER_CONFIG_WORD_GAP_SMALL = {50, 80}, like a % of font height allowed to use as letter spacing (because my 1px has no real effect when you're using a large font size).

Spacing change looks distinct enough, worth trying a 10-page read to check :)

You mean you appreciate the reduction of spaces width?
If you really want to get a feeling of it (with all the bugs included :)
libcrengine-kobo_hacky_letter_spacing1.zip

@NiLuJe
Copy link
Copy Markdown
Member

NiLuJe commented Apr 27, 2020

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

@NiLuJe
Copy link
Copy Markdown
Member

NiLuJe commented Apr 27, 2020

And, yeah, if possible, I'd be happy with that being a fancy toggle, too.

@poire-z
Copy link
Copy Markdown
Contributor Author

poire-z commented Apr 27, 2020

Did you get the Before/After wrong, by chance?

Indeed :/ Fixed.
"After"= 5d.png is the one that is losing hyphens and the indentation on the top line.

In any case, at a quick glance, I'm happy with !"Before" (i.e., ... 5d.png) ;).

You mean you like it?! With all the kerning 'n hinting being messed with? I'm a bit surprised :)

@NiLuJe
Copy link
Copy Markdown
Member

NiLuJe commented Apr 27, 2020

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

@mergen3107
Copy link
Copy Markdown
Contributor

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.
Will this be available in the upcoming nightly? I wonder how does it "stack" with those quotation marks related stuff (like moving quoted word to the next line if there is not enough space for the quotation mark)

@poire-z
Copy link
Copy Markdown
Contributor Author

poire-z commented Apr 28, 2020

Will this be available in the upcoming nightly?

Well, no :) That was just a quick try to get a feeling if it was worth going on doing it well :)

I wonder how does it "stack" with those quotation marks related stuff

Should go with that well.
My main idea was that we couldn't do this added letter spacing in the last phase of just positionning words in the line (for justification), beacuse let's say you have 15 extra px, you could just distribute 1px to the 15 first letters, so the 3-4 first words, and the next words wouldn't have that letter spacing, and it could feel strange.
So, it has to be done in the early phase of making lines: if with no added letter spacing, you have > 5% unused space (5% is a value crengine already uses as the trigger to try hyphenating next word), we would reprocess making this line with adding 1px letter spacing to all chars seen - and if still >5%, try adding 2px (up to some limit - and these loops might increase rendering time). So, line wrapping (which ensure no bad wrap on quotation marks) and hyphenation could happen at different points for each step.
And I guess if none of 0px, 1px, 2px... gave a satisfying result < 5%, we should redo it with 0px (or the one that gave the lowest %).
(And we should skip cursive words like arabic in that process.)
So, still need some thinking, work and testing :)

@poire-z
Copy link
Copy Markdown
Contributor Author

poire-z commented May 1, 2020

My main idea was that we couldn't do this added letter spacing in the last phase of just positionning words in the line

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 Word Spacing one. I initially thought combining it into Word Spacing, but it might hardcode our choice for this new setting, and people might not like that added spacing...
So, Word Stretching ?
But that will make this bottom config menu have 6 items, which might cover a bunch of text and keep less of it to see the effects of all the settings... Any idea for reorganising that bottom menu (the Font size menu has only 2... but not obvious what we could move here - and it would make it different from the PDF one).

Also, not sure about the metric to use/trigger/limit the allowed % of added letter spacing (% of what?).
For now, I'm with:

#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.
I'm showing on the right the letter spacing used, as a % of the font size (which are always quite discreet number 1px is often 3% or 4% of the font size, 2px is 7/8%, 3px is 12%). Limiting it to 5% can often just be above to prevent it from using 2px...

The following is with using that maxAddedLetterSpacingPercent as a 3rd setting of the word spacing menu:

+DCREREADER_CONFIG_WORD_SPACING_SMALL = {95, 75, 0}
+DCREREADER_CONFIG_WORD_SPACING_MEDIUM = {95, 75, 5}
+DCREREADER_CONFIG_WORD_SPACING_LARGE = {95, 75, 15}

So, none, max 5%, max 15%.

None:
image

Max 5%:
image

Max 15%:
image

None:
image

Max 5%:
image

Max 15%:
image

Not sure how much this is annoying with ligatures:
image

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):
libcrengine-kobo_hacky_letter_spacing2.zip
Just needs one line added into frontend:

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

And some adaptation (used your prefered value as the common base) in defaults.lua or defaults.persistent.lua:

DCREREADER_CONFIG_WORD_SPACING_SMALL = {95, 75, 0}
DCREREADER_CONFIG_WORD_SPACING_MEDIUM = {95, 75, 5}
DCREREADER_CONFIG_WORD_SPACING_LARGE = {95, 75, 15}

@ptrm
Copy link
Copy Markdown
Contributor

ptrm commented May 1, 2020

@poire-z

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

Thanks ^_^ That would make me able to share my two cents about it.

@NiLuJe
Copy link
Copy Markdown
Member

NiLuJe commented May 1, 2020

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

@NiLuJe
Copy link
Copy Markdown
Member

NiLuJe commented May 1, 2020

@poire-z: I'm always seeing multiples of two in the margins. Expected, or coincidence?

@poire-z
Copy link
Copy Markdown
Contributor Author

poire-z commented May 1, 2020

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

@poire-z
Copy link
Copy Markdown
Contributor Author

poire-z commented May 1, 2020

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 :)
We need this ratio to font size, for this to have some kinda similar effect with small and large font size.
Finishing some bits, and I'll PR this later today - so you can have a look, in case you get some other idea.

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

The numbers that you see are %, not px. So, if you see 2 4 6, it's probably 1px, 2px, 3px.

@poire-z
Copy link
Copy Markdown
Contributor Author

poire-z commented May 1, 2020

OK, proposed in koreader/crengine#341.
The interesting stuff is in lvtextfm.cpp in the 4th commit.

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.

kobo aura hd Chinese mobi gibberish

6 participants