sile-typesetter / sile

The SILE Typesetter — Simon’s Improved Layout Engine
https://sile-typesetter.org
MIT License
1.66k stars 98 forks source link

new version don`t work good in Arabic #1078

Closed hubaishan closed 4 years ago

hubaishan commented 4 years ago

image this is test/sura-2.sil

simoncozens commented 4 years ago

Whoa, that's not good. But it looks fine for me and our integration tests for that file are passing, so I think it must be something to do with your setup. Could you please run sile --debug=versions tests/sura-2.sil?

alerque commented 4 years ago

2 comments:

  1. This is a known bad test. It does not render correctly for me either, although quite curiously my output it is somewhat different from your display. @simoncozens are you sure the results of the test look okay? They look like the Arabic is a bit scrambled to me (it's full of collisions) and the page break thing from #192 is still very much a problem.

  2. As a general note about tests, it's worth noting that nothing in the tests/ folder is meant to be built manually using sile. They are all part of a test suite that assumes very specific versions of fonts. The build system knows how to run them by downloading exactly the right font files and using those instead of your system defaults.

    From the directory of SILE source, you can render this test to a preview PDF by running:

    $ make tests/sura-2.pdf

    (Make user you've done the bootstrap/configure/make steps first of course).

simoncozens commented 4 years ago

It's working fine for me, both before and after the page break.

Screenshot 2020-10-08 at 21 14 52

$ ./sile --debug=versions tests/sura-2.sil
"versions"This is SILE v0.10.9.r40-g61193a9-dirty
<tests/sura-2.sil>
[1] [2]
Harfbuzz version: 2.6.8
Shapers enabled: ot, coretext, fallback
ICU support enabled

Fonts used:
/Users/simon/Library/Fonts/amiri-quran.ttf:0    Version 000.109
/Users/simon/Library/Fonts/amiri-quran.ttf:0    Version 000.109
alerque commented 4 years ago

SILE v0.10.9.r40-g61193a9? There have been 188 commits and 2 releases since then ... plenty of time to introduce a regression.

I would bisect, but g61193a9 fails for me just as ugly as the current release & HEAD versions. It's garbage.

image

$ ./sile --debug=versions tests/sura-2.sil
"versions"SILE v0.10.11.r4-ge41e393 (Lua 5.4)
<tests/sura-2.sil>
[1] [2]
Harfbuzz version: 2.7.2
Shapers enabled: graphite2, ot, fallback
ICU support enabled

Fonts used:
/usr/share/fonts/TTF/AmiriQuran.ttf:0   Version 0.113

I wonder if Amiri Quran 0.113 has any Graphite tables. That might explain this.

alerque commented 4 years ago

Note the test system uses Amiri Quran 0.111. My system has a newer version, Simon's is older.

I'll note that the 0.111 as used in the test suite is causing the line wrap issue. v0.113 as on my system seems to fix that. The shaping is still scrambled eggs though.

Also forcing OT shaping with \set[parameter=harfbuzz.subshapers,value=ot] does not help.

hubaishan commented 4 years ago

` "versions"

SILE v0.10.11 (Lua 5.2)
<sura-2.sil>
[1] [2]
Harfbuzz version: 2.6.4
Shapers enabled: graphite2, ot, fallback
ICU support enabled

Fonts used:
/usr/share/fonts/opentype/fonts-hosny-amiri/AmiriQuran.ttf:0     V e r s i o n   0 0 0 . 1 1 2
/usr/share/fonts/opentype/fonts-hosny-amiri/AmiriQuran.ttf:0     V e r s i o n   0 0 0 . 1 1 2
simoncozens commented 4 years ago

OK, I can replicate this on my Linux machine. Will check out what's going on.

simoncozens commented 4 years ago

Hrm, no, it is a regression. HEAD is failing on my OS X machine too.

simoncozens commented 4 years ago

7caa9c82 is good. 095e4ac is bad. Setting a string with a displacement (x_offset/y_offset) should not adjust the cursor.

alerque commented 4 years ago

I'll have a look. I'm pretty sure backing that out is going to break rule placement in some direction(s).

simoncozens commented 4 years ago

I've got it.

alerque commented 4 years ago

Thanks for reporting this @hubaishan. The fix for this is in master now so you can use a source build. This is enough of a regression that I'll try to expedite the next release so it gets into prebuilt packages sooner rather than later, but it still might be a couple days.

Also note that this is not a problem for all Arabic, only some fonts have features that trigger this bug.

simoncozens commented 4 years ago

It’ll be a problem for anything that uses either cursive positioning (many Arabic fonts) or mark placement (any tashkil and many ijam). So, yeah, this is Really Bad for Arabic. Thanks for sorting out a quick bug fix release. And we clearly need more Arabic tests.