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

jk/glyph indices #2139

Merged
merged 25 commits into from
Aug 24, 2022
Merged

jk/glyph indices #2139

merged 25 commits into from
Aug 24, 2022

Conversation

jkrumbiegel
Copy link
Member

  • switch from chars to Culong glyph indices
  • save glyph indices in glyphcollection
  • draw glyphs by index in cairomakie
  • remove new path
  • add stub for MathTeXEngine

@MakieBot
Copy link
Collaborator

MakieBot commented Jul 14, 2022

Compile Times benchmark

Note, that these numbers may fluctuate on the CI servers, so take them with a grain of salt. All benchmark results are based on the mean time and negative percent mean faster than the base branch. Note, that GLMakie + WGLMakie run on an emulated GPU, so the runtime benchmark is much slower. Results are from running:

using_time = @ctime using Backend
# Compile time
create_time = @ctime fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @ctime Makie.colorbuffer(display(fig))
# Runtime
create_time = @benchmark fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @benchmark Makie.colorbuffer(display(fig))
using create display create display
GLMakie 16.11s (15.91, 16.35) 0.18+- 20.35s (20.17, 20.51) 0.15+- 20.70s (20.21, 21.03) 0.28+- 16.55ms (16.29, 16.77) 0.18+- 85.06ms (83.30, 86.88) 1.30+-
master 16.19s (15.84, 16.46) 0.25+- 20.13s (19.90, 20.37) 0.16+- 20.43s (20.19, 20.65) 0.16+- 16.83ms (16.58, 17.37) 0.26+- 84.58ms (83.89, 85.14) 0.49+-
evaluation -0.51%, -0.08s invariant (-0.37d, 0.50p, 0.22std) +1.09%, 0.22s slower X (1.44d, 0.02p, 0.15std) +1.28%, 0.27s invariant (1.16d, 0.06p, 0.22std) -1.69%, -0.28ms faster ✓ (-1.24d, 0.04p, 0.22std) +0.56%, 0.48ms invariant (0.48d, 0.39p, 0.89std)
CairoMakie 11.77s (11.68, 11.86) 0.06+- 19.87s (19.76, 19.94) 0.07+- 2.50s (2.49, 2.51) 0.01+- 14.59ms (14.49, 14.66) 0.05+- 25.22ms (25.13, 25.39) 0.09+-
master 11.79s (11.76, 11.83) 0.02+- 19.84s (19.76, 19.86) 0.04+- 2.49s (2.49, 2.50) 0.00+- 14.99ms (14.92, 15.03) 0.04+- 25.17ms (24.91, 25.45) 0.19+-
evaluation -0.22%, -0.03s invariant (-0.62d, 0.28p, 0.04std) +0.16%, 0.03s invariant (0.59d, 0.30p, 0.05std) +0.30%, 0.01s slower X (1.73d, 0.01p, 0.00std) -2.79%, -0.41ms faster ✓ (-8.26d, 0.00p, 0.05std) +0.21%, 0.05ms invariant (0.35d, 0.53p, 0.14std)
WGLMakie 24.39s (23.33, 25.32) 0.70+- 41.31s (39.55, 42.79) 1.20+- 64.70s (63.09, 65.73) 1.00+- 35.77ms (34.99, 36.90) 0.67+- 2.07s (2.05, 2.11) 0.02+-
master 24.94s (24.13, 25.78) 0.58+- 42.69s (40.86, 44.47) 1.47+- 64.76s (61.14, 67.50) 2.47+- 35.21ms (33.98, 37.14) 1.03+- 2.03s (1.99, 2.07) 0.03+-
evaluation -2.28%, -0.56s invariant (-0.86d, 0.13p, 0.64std) -3.34%, -1.38s invariant (-1.03d, 0.08p, 1.34std) -0.10%, -0.06s invariant (-0.03d, 0.95p, 1.74std) +1.55%, 0.56ms invariant (0.64d, 0.26p, 0.85std) +1.71%, 0.04s slower X (1.43d, 0.02p, 0.02std)

@Kolaru
Copy link
Contributor

Kolaru commented Jul 14, 2022

@jkrumbiegel It doesn't seem to work, after a quick (and terribly hacky) test. Despite passing the correct font and the correct index in the stub you provide, nothing appear when I use the text function.

I tested it by using glyph index 5930 in the NewCMMath-Regular font (from the NewComputerModern family), which should contain an integral symbol, as confirmed by http://fontdrop.info :
image

Unfortunately I couldn't quite understand how to select font in Cairo directly, so I couldn't test if it works in pure Cairo.jl. I'll have another try tomorrow.

Comment on lines +460 to +462
ccall((:cairo_show_glyphs, Cairo.libcairo),
Nothing, (Ptr{Nothing}, Ptr{CairoGlyph}, Cint),
ctx.ptr, cg, 1)
Copy link
Member

@asinghvi17 asinghvi17 Jul 15, 2022

Choose a reason for hiding this comment

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

In this case you might use cairo_show_text_glyphs, which allow text to be highlighted. We had glyph support earlier but AFAIK the text highlighting issue is why we switched away from it. It would also allow the whole GlyphCollection to be rendered at once.

Copy link
Member Author

Choose a reason for hiding this comment

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

At this point I don't have the utf 8 anymore and it's not guaranteed that there is any, as for Mathtexengine

@jkrumbiegel
Copy link
Member Author

For me it worked when I used the computer modern font and just spliced in the value 5390 for the glyph in the CairoMakie drawing code. Just like that:

function show_glyph(ctx, glyph::Culong, x, y)
    cg = Ref(CairoGlyph(5930, x, y))
    ccall((:cairo_show_glyphs, Cairo.libcairo),
            Nothing, (Ptr{Nothing}, Ptr{CairoGlyph}, Cint),
            ctx.ptr, cg, 1)
end
cm = Makie.FTFont("~/.julia/packages/MathTeXEngine/XFmQJ/assets/fonts/NewComputerModern/NewCMMath-Regular.otf")

s = Scene(camera = campixel!)
text!(s, 100, 100, text = "hi", font = cm)
s

image

So it's definitely possible accessing the glyph. However, what I first did didn't work, returning 5930 from the place where I introduced the error for you. But I assume now that this was because MathTeXEngine actually selected the wrong font for the expression that I did, which was just L"x+y".

@Kolaru
Copy link
Contributor

Kolaru commented Jul 15, 2022

Okay it is on me, I asked too much from Revise. It seems to be all good, and I can work on setuping MathTeXEngine to work with that.

@jkrumbiegel
Copy link
Member Author

Great, I think it should be sufficient that TeXChar carries the Culong and then I can build it in from Makie's side. Would need to be a minor version change anyway right? So no compat problem until Makie is ready to integrate

@Kolaru
Copy link
Contributor

Kolaru commented Jul 16, 2022

Yes, on MathTeXEngine side it is breaking, since the font will slightly change as well as the API of TeXChar (slightly).

@jkrumbiegel
Copy link
Member Author

I'm making some changes to FreeTypeAbstraction in this PR, you can probably just use the functions there for getting glyph indices instead of duplicating that on your side JuliaGraphics/FreeTypeAbstraction.jl#72

Copy link
Contributor

@Kolaru Kolaru left a comment

Choose a reason for hiding this comment

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

With these two changes, it is compatible with Kolaru/MathTeXEngine.jl#67

The MathTeXEngine PR will require a bit more work to make sure everything work as it should, but the API shouldn't change.

CairoMakie/src/primitives.jl Outdated Show resolved Hide resolved
src/basic_recipes/text.jl Outdated Show resolved Hide resolved
@jkrumbiegel
Copy link
Member Author

Ok @Kolaru I could use your branch locally, the integral for example looks like this:

image

Seems like there's some wrong boundingbox information on that though?

Anyway, you can further test your changes with this branch, then when you tag MathTeXEngine it should be picked up by this PR. I've already bumped compat.

@jkrumbiegel
Copy link
Member Author

Hm currently the only thing still problematic is that the word wrapping checks for \n but I'm only storing glyph indices now. Any opinions @ffreyer what would be a good reimplementation? Maybe the characters should still be stored side by side, but that's not really possible or meaningful for MathTeXEngine

@jkrumbiegel
Copy link
Member Author

Actually I'm not sure if word wrapping makes much sense for MathTeXEngine at all..

@ffreyer
Copy link
Collaborator

ffreyer commented Aug 21, 2022

Actually I'm not sure if word wrapping makes much sense for MathTeXEngine at all..

I think it does. If you have some formulas you may want other text as latex strings for visual cohesion.

Hm currently the only thing still problematic is that the word wrapping checks for \n but I'm only storing glyph indices now. Any opinions @ffreyer what would be a good reimplementation? Maybe the characters should still be stored side by side, but that's not really possible or meaningful for MathTeXEngine

Can't you just compare to glyph_index('\n', font) instead?

@jkrumbiegel
Copy link
Member Author

Can't you just compare to glyph_index('\n', font) instead?

True, didn't think this way around.

Hm yeah maybe, I haven't used latexstrings much and especially not at sizes that require word wrapping.

@jkrumbiegel jkrumbiegel marked this pull request as ready for review August 22, 2022 18:10
@jkrumbiegel jkrumbiegel merged commit a192fa7 into master Aug 24, 2022
@jkrumbiegel jkrumbiegel deleted the jk/glyph-indices branch August 24, 2022 10:57
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.

5 participants