sile icon indicating copy to clipboard operation
sile copied to clipboard

libtexpdf draws glyphs at wrong positions, depending on circumstances

Open OlivierNicole opened this issue 3 years ago • 20 comments

How to reproduce: checkout revision https://github.com/OlivierNicole/sile/commit/280e43f76fb8b465732804ac95266e41e2276576. I have not been able to reproduce this on mainline SILE, yet I suspect the problem is in there too, for the reason exposed below.

Compile the following document:

\begin[class=plain]{document}

\set[parameter=math.font.family, value=Libertinus Math]
\set[parameter=math.font.size, value=16]

\begin{center}

\begin[mode=display]{math}
  (f)
\end{math}

\end{center}

\end{document}

Expected output: image

Actual output: image

The reason I think the bug is not (or not only) in my code is that, if you modify SILE as such:

diff --git a/core/libtexpdf-output.lua b/core/libtexpdf-output.lua
index 2668a294..d126ce58 100644
--- a/core/libtexpdf-output.lua
+++ b/core/libtexpdf-output.lua
@@ -110,6 +110,8 @@ SILE.outputters.libtexpdf = {
   end,
 
   _drawString = function(self, str, width, x_offset, y_offset)
+    pdf.colorpush_rgb(0,0,0)
+    pdf.colorpop()
     local x, y = self:getCursor()
     pdf.setstring(x+x_offset, y+y_offset, str, string.len(str), self._font, width)
   end,

then, you obtain the correct output (”expected output” above). This is obviously puzzling, because these two lines of code should be a no-op.

I have looked briefly at the src/justenoughlibtexpdf.c without finding clues.

What the math command does is calling SILE.typesetter:pushHbox() on a nodefactory.box which contains smaller, nested boxes, each with its shape() and outputYourself() functions.

I read somewhere that the PDF output routine supports kerning by doing some smart things with glyph advances, so maybe it might be related?

OlivierNicole avatar Jan 16 '21 13:01 OlivierNicole

This bug surely has something to do with the (awkward) fact that the cursor location is being tracked in two different places (once in the PDF rendering library and once or our own internally) and each of these get their druthers sorted out at different points.

alerque avatar Jan 27 '21 12:01 alerque

I just tried to get a situation together to replicate this and can't quite get there. Commit 280e43f seems to be lacking some files (perhaps they were present but untracked in your working tree) in order for math to work. I tried the tip of your math branch and that seems better, but the MWE in the issue doesn't work as expected there. I tried adding \script[src=packages/math] to the example code, but that doesn't fly either. Any tips?

alerque avatar Jan 27 '21 12:01 alerque

Sorry about that. You can remove lines 915 and 916 of file packages/math/base-elements.lua and the MWE should be ugly again.

OlivierNicole avatar Jan 27 '21 13:01 OlivierNicole

Sorry I wasn't more clear. It wasn't that I couldn't reproduce the typesetting issue, it's that I couldn't even start to render it.

As posted, the MWE code errors with:

$ ./sile foo.sil
SILE v0.10.12.r125-g280e43f (Lua 5.4)
<foo.sil>

! Undefined setting 'math.font.family' at foo.sil:4:1: in \settable: 0x55f5f18434e0

With the \script[src=package/math] attempt I expected to work:

./sile foo.sil
SILE v0.10.12.r125-g280e43f (Lua 5.4)
<foo.sil>

Error detected:
        error loading module 'packages/math' from file 'packages/math':
        cannot read packages/math: Is a directory

alerque avatar Jan 27 '21 13:01 alerque

On the plus side, make docs works in the math branch and the math in the manual looks great! So clearly we aren't too far away.

alerque avatar Jan 27 '21 13:01 alerque

I am surprised, \script[src=packages/math] seems to load packages/math.lua correctly for me. I will try a complete reinstall when I have access to my computer again.

OlivierNicole avatar Jan 27 '21 13:01 OlivierNicole

I cannot reproduce your error. At the tip of the math branch (https://github.com/OlivierNicole/sile/commit/9d49c2ec6ce573e1a63c43e62a365439f59ee65f), with \script[src=packages/math], I have other issues because I don't have a math font on this machine, but the package loads fine.

OlivierNicole avatar Jan 27 '21 15:01 OlivierNicole

@OliverNicole My error was related to Lua path priority weirdness. Your example works fine from anywhere you run it except the SILE project root directory. Which is where I was playing around.

alerque avatar Jan 28 '21 12:01 alerque

A couple comments while trying to reproduce this:

  • Why is the example above in a \center block? \math[mode=display] seems to be centering on it's own.
  • When I try to update the math branch with a fresh merge from master, all the math positioning seems to work but the glyphs are tofo boxes. Any clues?
  • There is some new UTF-8 related LGEP grammar bits. Given that we've just recently (not released yet but in master) started depending on an external library to help shim UTF-8 support, I'm wondering it we shouldn't do the same for this. I remember bumping into UTF-8 related LPEG modifications. Did you by chance look into what's out there?

Also I'm having trouble with that PR. Usually when GitHub says I can add commits by pushing to the branch I can. For some reason in this case I can't. Not likely your fault, just know that's why I haven't fixed the font in the docs issue. That is as simple as this commit which I can't seem to push:

From 6d01cf8585829bb8f5d0ac5119eb9a9c3fbc66fd Mon Sep 17 00:00:00 2001
From: Caleb Maclennan <[email protected]>
Date: Wed, 27 Jan 2021 16:23:24 +0300
Subject: [PATCH] chore(build): Require math font to build manual

Signed-off-by: Caleb Maclennan <[email protected]>
---
 Makefile-fonts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile-fonts b/Makefile-fonts
index 587c6f64..26bf98fe 100644
--- a/Makefile-fonts
+++ b/Makefile-fonts
@@ -9,6 +9,7 @@ DOCSFONTFILES += CormorantInfant-Italic.ttf CormorantInfant-Regular.ttf
 DOCSFONTFILES += GenBkBasR.ttf GenBkBasI.ttf GenBkBasB.ttf
 DOCSFONTFILES += Hack-Regular.ttf
 DOCSFONTFILES += LateefGR-Regular.ttf
+DOCSFONTFILES += LibertinusMath-Regular.otf
 DOCSFONTFILES += LibertinusSerif-Regular.otf
 DOCSFONTFILES += NotoSansCJK-Regular.ttc
 DOCSFONTFILES += NotoSansEthiopic-Regular.ttf
-- 
2.30.0

alerque avatar Jan 28 '21 13:01 alerque

  1. You are perfectly right, that slipped my mind.
  2. Oops… no clues yet, I will bisect.
  3. Is there a changelog for the LPEG library somewhere? I can’t seem to find one. But I am 100 % up for using functions from a dedicated library. The only reason I added UTF-8-related stuff is because contrary to SILE’s, the TeX-like parser needs to be able to single out meaningful characters, which it turns into “atoms” (e.g. “xy” becomes “atom x, atom y”).

OlivierNicole avatar Jan 28 '21 18:01 OlivierNicole

Regarding pushing on my PR branch, I have no clue, but it looks like you just solved it.

OlivierNicole avatar Jan 28 '21 18:01 OlivierNicole

Regarding pushing on my PR branch, I have no clue, but it looks like you just solved it.

Sadly, no. I was able to edit the file and commit from GitHub's built in editor on the PR. That proves I should have access to push commits to the branch, but I still can't do that. Good news is it's likely not your fault, its almost certainly a GitHub bug.

alerque avatar Jan 28 '21 18:01 alerque

As a workaround, I can give you developer rights on the repo.

OlivierNicole avatar Jan 28 '21 18:01 OlivierNicole

Bisecting pointed dd95b1d54c9cc2e36251343e77c3b7423e72cb62 as responsible for the “glyph not found” boxes. I suppose there is a subtle difference between the luautf8 functions and the functions that you deprecated. Will investigate further later.

OlivierNicole avatar Jan 28 '21 19:01 OlivierNicole

Oddly I don't have a dd95b1d5 (and neither does GitHub, anywhere in any repo). I assume you mean 934b8372. I can confirm reverting that fixes the TOFO. Furthermore the SU.utf8char()luautf8.char() change is fine, it is specifically the SU.utf8codes()luautf8.codes() swap that is problematic for the math stuff.

alerque avatar Jan 28 '21 19:01 alerque

Found the trouble with the UTF8 functions. Fix pushed. Now back to debugging what this issue was originally about.

alerque avatar Jan 28 '21 19:01 alerque

This is bizarre. So far I'm stumped.

-- works to fix positioning, as does any > 0 value
pdf.colorpush_gray(1)
pdf.colorpop()

--- doesn't work
pdf.colorpush_gray(0)
pdf.colorpop()

Also doing the push/pop before or after the hbox draw has no effect, it works either way.

Just pushing a color doesn't work, you have to push/pop.

alerque avatar Jan 28 '21 20:01 alerque

Thank you for the fix!

Oddly I don't have a dd95b1d5 (and neither does GitHub, anywhere in any repo). I assume you mean 934b8372.

Yes, sorry! Forgot I’d done a rebase in order to bisect, so… new commit hashes.

OlivierNicole avatar Jan 28 '21 21:01 OlivierNicole

I am skimming through libtexpdf’s code, to no avail. This library is not a piece of cake.

OlivierNicole avatar Jan 30 '21 22:01 OlivierNicole

I wish we'd found this before this release cycle, but I haven't been able to make sense of it yet. In any case this is (so far) only evidenced with the upcoming math support, so it's not the end of the world that it doesn't make today's cut.

alerque avatar Feb 03 '21 17:02 alerque