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

Fix compatibility issue with MathTeXEngine #1785

Closed
wants to merge 17 commits into from

Conversation

Kolaru
Copy link
Contributor

@Kolaru Kolaru commented Mar 25, 2022

Description

Fixes the compatibility problem discussed in #1491 using the changes introduced in MathTeXEngine in Kolaru/MathTeXEngine.jl#58

I have given the same interface to TeXChar that FreeTypeAbstraction.FontExtent has. This means TeXChar can be used interchangeably with FontExtent, putting the full responsability to produce correct bounding boxes and positioning on MathTeXEngine without requiring additional special cases for LaTeXString in Makie.

I run the test locally and everything looked fine. Hopefully I did it correctly.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Edit: Looks like it may have been wise to wait for the registration of the new version of MathTeXEngine to be fully register before using it here >_<

@Kolaru Kolaru changed the title Use TeXChar common interface with FontExtent Fix compatibility issue with MathTeXEngine Mar 25, 2022
@Kolaru
Copy link
Contributor Author

Kolaru commented Mar 25, 2022

The new version of MathTeXEngine has been merged to the registry so the CI can be retriggered (not sure if I can do it directly).

@Kolaru
Copy link
Contributor Author

Kolaru commented Mar 28, 2022

(so that the way to do it... thanks Simon ^^')

The tests fail because of one reference test:

image

Essentially the sum symbol got a little bit bigger and the big square root symbol changed.

To me it looks okay (but I don't quite understand how to update the reference or even if I need to do it manually).

So I would say it is ready for review.

@jaakkor2
Copy link
Contributor

Square root looks much better now.

Y-label code would be better as
ylabel = L"x + y - \sin(x) × \tan(y) + \sqrt{2}",, i.e., backslash sin and tan.
https://github.com/JuliaPlots/Makie.jl/blob/432b8ab0b560e9acc0a2da4f1f0dddeadd3ae7c2/ReferenceTests/src/tests/text.jl#L257

@SimonDanisch
Copy link
Member

Closes #1785 I suppose

src/types.jl Outdated
@@ -359,7 +359,7 @@ struct GlyphCollection
glyphs::Vector{Char}
fonts::Vector{FTFont}
origins::Vector{Point3f}
extents::Vector{FreeTypeAbstraction.FontExtent{Float32}}
extents::Union{Vector{FreeTypeAbstraction.FontExtent{Float32}}, Vector{TeXChar}}
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we use the same representation for both? It doesn't have to be FontExtent, but that is just a collection of a couple numbers anyway, so those should be extractable from TeXChar. Basically at the later point in the pipeline, I would like to not differentiate between different ways to generate text anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I am lazy wanted to minimize changes for that PR.

That can totally be handled by the the GlyphCollection constructor and then stored in an unique way. I will look into it.

@jonas-schulze
Copy link
Contributor

Would this help with #1560?

@SimonDanisch
Copy link
Member

What's the state of this? Looks like most issues are addressed?
I'm seeing this in the reference image tests, which looks more like an improvement then a regression:
latex
I measured the pixel distance from the border to the font, and before there is 1pixel difference between x/y, while now it's around 0.5 pixel difference, or something like that.

@Kolaru
Copy link
Contributor Author

Kolaru commented Apr 25, 2022

I think there is just a very minor change in the computation of the bounding box, but I haven't have time to go through all the references images to make sure I haven't break anything (I preferred to wait for the CI to make sure the tests are run correctly).

I will ping you once I have gone through everything.

@piever
Copy link
Contributor

piever commented May 12, 2022

Sorry to bump here, but is this ready to be merged?

I suspect this may fix some problems in AlgebraOfGraphics: updating to new Makie somehow confuses the resolver, as the old MathTexEngine brings in an old version of RelocatableFolders, see here. I imagine upgrading to newer MathTexEngine would fix that.

@SimonDanisch
Copy link
Member

@Kolaru happy to help getting this over the finish line!

@ffreyer
Copy link
Collaborator

ffreyer commented May 13, 2022

I think one of my recent pr's messed up something with lines here. I'll try to merge master and fix it

@ffreyer
Copy link
Collaborator

ffreyer commented May 13, 2022

Some nitpicking:

Fraction lines look slightly too far left to me:
fraction

The bounding box of integrals is a bit too tight:
bounding box

@ffreyer
Copy link
Collaborator

ffreyer commented May 13, 2022

I looked through the GLMakie failures. As far as I can tell they all failed because the bottom white space is slightly less, like in Simons gif. I don't think it's a problem...

@ffreyer
Copy link
Collaborator

ffreyer commented May 13, 2022

height_insensitive_boundingbox_with_advance was using a ymax instead of a height. With that fixed:

Before
After

However using ink_bounding_box gives even tighter bounding box, so I switched to that: (This also works for normal text)

ink bounding box

@SimonDanisch
Copy link
Member

Nice, thank you! :)
Looks good so far, besides this one, which seems a bit tight around the x label:
image

@ffreyer
Copy link
Collaborator

ffreyer commented May 13, 2022

Failures (in GLMakie) are still related to bounding box changes. Most are irrelevant but some are worth mentioning:

It seems like something changes about axis limits in this test:

Screenshot from 2022-05-13 16-27-53
Screenshot from 2022-05-13 16-27-55

Latex ticks (and latex labels) are very tight now: (Though you might count that as a problem of normal text, since that seems to use more space than it technically should)

Screenshot from 2022-05-13 16-30-01
Screenshot from 2022-05-13 16-30-03

@ffreyer
Copy link
Collaborator

ffreyer commented May 13, 2022

I changed text alignment to be based on character sizes rather than font sizes. As far as I can tell this makes the vertical alignment of normal text gap-less now. I tweaked the gaps in Axis to get back to how they are on master. There is probably a bunch more that need a ~2px bump because of these changes though...
Screenshot from 2022-05-13 22-42-03

@SimonDanisch
Copy link
Member

image

@ffreyer Could they be more accurate now, so that 7f0 is actually closer to the padding we want?

@SimonDanisch
Copy link
Member

What's with this?
image

Reference:
image

@ffreyer
Copy link
Collaborator

ffreyer commented May 14, 2022

Could they be more accurate now, so that 7f0 is actually closer to the padding we want?

The left/right alignment still has a gap between text.position[] and the start/end of a character. I think that's true for both types of strings though, so I didn't try to change it here. I changed the ylabelpadding because the rotated label lost about 2px with the change in vertical alignment.

What's with this?

The tallest character starts/ends at the y value of the given text position now. 3 of those share that position, so they touch now. Before it was based on the height of the font, which I guess means the tallest character in the font?

@ffreyer
Copy link
Collaborator

ffreyer commented May 14, 2022

I think these tight alignment changes are also part of #1847 (but probably implemented differently) Maybe we can merge the prs?

@jkrumbiegel
Copy link
Member

The reason that I don't use the exact character bounding boxes anywhere else are that when you animate a title for example, you don't want it to jitter up and down when you change an f to a g and I use the advance values for left/right alignment because otherwise left-aligned text for example looks more "ragged". It is a bit more difficult with MathTeXEngine to choose a good behavior because the ascender / descender settings of the font used seem pretty large.

@jkrumbiegel
Copy link
Member

Obsolete via #1952

@SimonDanisch
Copy link
Member

Thanks everyone :) Really happy to get this finally merged :)

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

7 participants