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

Fixed invalid text measurements when using HarfBuzz and in used text … #29

Merged
merged 1 commit into from
Aug 7, 2018
Merged

Fixed invalid text measurements when using HarfBuzz and in used text … #29

merged 1 commit into from
Aug 7, 2018

Conversation

virxkane
Copy link
Collaborator

@virxkane virxkane commented Aug 7, 2018

Fix for my own kerning implementation, sorry for broken code...
Fixed invalid text measurements when using HarfBuzz and in used text font found any ligatures (for example, "Noto Serif").
The key error: fill char's flags at invalid position:

-flags[i] = GET_CHAR_FLAGS(ch);
+flags[cluster] = GET_CHAR_FLAGS(ch);

Invalid:
screenshot_20180807_080008
Fixed:
screenshot_20180807_102410

…font found any ligatures (for example, "Noto Serif").

The key error: fill char's flags at invalid position:

-flags[i] = GET_CHAR_FLAGS(ch);
+flags[cluster] = GET_CHAR_FLAGS(ch);
@virxkane
Copy link
Collaborator Author

virxkane commented Aug 7, 2018

Picture with explanations:
screenshot_20180807_080008 m

@buggins buggins merged commit 8985559 into buggins:master Aug 7, 2018
@virxkane virxkane deleted the harfbuzz-kerning-fix branch August 7, 2018 10:10
@Frenzie Frenzie mentioned this pull request Aug 7, 2018
poire-z added a commit to poire-z/crengine that referenced this pull request Aug 17, 2018
Picked the only relevant changes (crengine/*) from upstream PRs:
    buggins/coolreader#24
    buggins/coolreader#29
Fixed a single rejection due to our soft-hyphens support changes.
poire-z added a commit to koreader/crengine that referenced this pull request Aug 17, 2018
Picked the only relevant changes (crengine/*) from upstream PRs:
    buggins/coolreader#24
    buggins/coolreader#29
Fixed a single rejection due to our soft-hyphens support changes.
@poire-z
Copy link
Contributor

poire-z commented Jan 13, 2019

Just reporting some related additional fixes for stuff like this, as I got because of ligatures:
image

Got that on koreader, and we do additional stuff in lvtextfm.cpp, so dunno if you would get that too with coolreader, but I think you would.
It happened because of bad flags at end of measureText segment, because the bold node make out another segment to measure. Bad flags at the end of the first segment were causing this.

No real patch, just some extracts from mine, around that code in this PR:

             uint32_t j;
             uint32_t cluster;
             uint32_t prev_cluster = 0;
+            int skipped_chars = 0; // to add to 'i' at end of loop, as 'i' is used later and should be accurate
             for (i = 0; i < (int)glyph_count; i++) {
                 for (j = prev_cluster + 1; j < cluster; j++) {
                     // fill flags and widths for chars skipped (because they are part of a
                     // ligature and are accounted in the previous cluster)
                    flags[j] = GET_CHAR_FLAGS(text[j]);
                    widths[j] = prev_width;            // for chars replaced by ligature
+                    skipped_chars++;
                 }
                 for (j = prev_cluster + 1; j < (uint32_t)len; j++) {
                     flags[j] = GET_CHAR_FLAGS(text[j]);
                     widths[j] = prev_width;
+                    skipped_chars++;
                 }
             }
+            // i is used below to "fill props for rest of chars", so make it accurate
+            i += skipped_chars;
         // fill props for rest of chars
         for ( int ii=i; ii<len; ii++ ) {
-            flags[i] = GET_CHAR_FLAGS( text[ii] );
+            flags[ii] = GET_CHAR_FLAGS( text[ii] );
         }

Hope it will save you the many hours it took me to find out that last typo :) (guess that code didn't get triggered before, but is now with harfbuzz as, with skipped chars, there are now rest of chars to fill).

@virxkane
Copy link
Collaborator Author

@poire-z Ok, thank you, I fix this later.

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.

3 participants