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

Kolaru compat with mathtexengine #1952

Merged
merged 28 commits into from
May 20, 2022
Merged

Conversation

SimonDanisch
Copy link
Member

@SimonDanisch SimonDanisch commented May 18, 2022

Looked at #1785 today with @jkrumbiegel and using height_insensitive_boundingbox for chars seemed rather important for the rest of the text pipeline.

It doesn't seem to solve it though, since the M and O isn't centered correctly with height_insensitive_boundingbox:
image

It's centered correctly with inkboundingbox, but then the (:left, :bottom) is wrong, since the M shouldn't change positions depending on whether the g is present:
image

test code:

begin
    f = Figure()
    for (i, str) in enumerate(["M", "gM", "O"])
        for (j, align) in enumerate([(:left, :baseline), (:left, :bottom), (:center, :center)])
            ax, pl = scatter(f[i, j], Point2f(0, 0), axis=(title="$align",))
            text!(str, position = Point2f(0, 0), textsize = 50, align = align)
            hidedecorations!(ax)
        end
    end
    f
end

src/types.jl Outdated
@@ -319,7 +319,7 @@ end

function GlyphExtent(font, char)
extent = get_extent(font, char)
ink_bb = FreeTypeAbstraction.inkboundingbox(extent)
ink_bb = FreeTypeAbstraction.height_insensitive_boundingbox(extent, font)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes bounding boxes less accurate. Why not undo my changes instead? (use character height instead of font height)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is that way on purpose. text should not be bounding-boxed tightly around the glyphs, but given the font's ascenders and descenders. unless there is a specific purpose for having a tighltly bounded glyph, but normal text is not that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the question is just if LaTeX should use something else because text is positioned all over the place there. I don't know how real LaTeX handles equation bounding boxes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ok I guess I misunderstood, the ink_bb should of course be the inkboundingbox, but that doesn't mean we have to use it to compute text boundingboxes. it's still useful to have in that struct

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another direction I thought about going with my changes was to use something like overallheight = max(font_height, max_height_of_line) for latex. I guess that would be the way if we want to keep text aligned based on the largest character/font rather than individual characters.

Maybe we should also just have both styles for alignments and bounding boxes. I.e. one based on characters and one based on the (if necessary padded) font (height)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ideally the font ascender / descenders shouldn't be that far off from normal strings that have the occasional f or g. Just some fonts define those metrics in a peculiar way (case in point, the need for TeX Gyre Heros Makie). So only if you were to do something where absolute precision of boundingboxes of strings was necessary would you need those other styles. I think in all other situations the non-jitteryness is preferable and seems to be what other text-based tools or browsers etc are doing.

@MakieBot
Copy link
Collaborator

MakieBot commented May 18, 2022

Compile Times benchmark

Note, that these numbers may fluctuate on the CI servers, so take them with a grain of salt.

using time

master:  9.41 < 9.75 > 10.76, 0.62+-
pr:      9.55 < 9.73 > 10.71, 0.50+-
speedup: 0.98 < 1.00 > 1.10, 0.04+-
median:  -0.26% => invariant

This PR does not change the using time.

ttfp time

master   23.53 < 24.32 > 27.07, 1.64+-
pr       23.63 < 24.29 > 26.91, 1.40+-
speedup: 0.97 < 1.00 > 1.12, 0.05+-
median:  -0.12% => invariant

This PR does not change the ttfp time.

@jkrumbiegel
Copy link
Member

With this I get
grafik

@jkrumbiegel jkrumbiegel merged commit 182323e into master May 20, 2022
@jkrumbiegel jkrumbiegel deleted the Kolaru-compat_with_mathtexengine branch May 20, 2022 12:17
@Kolaru
Copy link
Contributor

Kolaru commented May 22, 2022

Thanks everyone who stepped up to finish that! I was on vacation for the last two weeks and couldn't contribute.

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.

None yet

5 participants