libtexpdf draws glyphs at wrong positions, depending on circumstances

OlivierNicole commented 3 years ago

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:

Actual output:

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?

alerque commented 3 years ago

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 commented 3 years ago

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?

OlivierNicole commented 3 years ago

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

alerque commented 3 years ago

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 commented 3 years ago

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.

OlivierNicole commented 3 years ago

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 commented 3 years ago

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.

alerque commented 3 years ago

@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 commented 3 years ago

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 <caleb@alerque.com>
Date: Wed, 27 Jan 2021 16:23:24 +0300
Subject: [PATCH] chore(build): Require math font to build manual

Signed-off-by: Caleb Maclennan <caleb@alerque.com>
---
 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

OlivierNicole commented 3 years ago

You are perfectly right, that slipped my mind.
Oops… no clues yet, I will bisect.
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 commented 3 years ago

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

alerque commented 3 years ago

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.

OlivierNicole commented 3 years ago

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

OlivierNicole commented 3 years ago

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.

alerque commented 3 years ago

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 commented 3 years ago

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

alerque commented 3 years ago

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.

OlivierNicole commented 3 years ago

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 commented 3 years ago

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

alerque commented 3 years ago

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.

Omikhleia commented 1 month ago

As of SILE 0.15.5

\begin[papersize=a7]{document}
\use[module=packages.math]
\begin{center}
\begin[mode=display]{math}
  (f)
\end{math}
\end{center}
\end{document}

Moreover: Same result removing the two weird lines regarding the color stack.

These lines were added in 6a4b20d ... without a FIXME or HACK in-code to explain the weird operations...

And the commit log does not seem to make sense either: "... resulting in SILE crashing whenever typesetting math with a backend other than libtexpdf." = With a backend other than libtexpdf, but we "fix" libtexpdf ????

alerque commented 1 month ago

With a backend other than libtexpdf, but we "fix" libtexpdf ????

This bit makes sense. The code was fixed by moving an action that only the libtexpdf backend is expected to support (hard coded access to its internals) from a generic location that would have been sent to all backends into the libtexpdf backend itself where it is expected to work.

sile-typesetter / sile

libtexpdf draws glyphs at wrong positions, depending on circumstances #1121