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

allow FTFont in TextConfig(font=...) #202

Merged
merged 4 commits into from
Feb 7, 2022

Conversation

ederag
Copy link
Contributor

@ederag ederag commented Jan 15, 2022

As discussed in #194, with this PR, TextConfig accepts also an FTFont, bypassing the findfont call.

julia> @time gif_output = GifOutput(init;
                  filename="forestfire.gif", tspan=1:200, fps=25,
                  minval=DEAD, maxval=BURNING,
                  scheme=ColorSchemes.rainbow,
                  zerocolor=RGB24(0.0),
                  font="Cantarell"
)
  2.943864 seconds (313.00 k allocations: 155.443 MiB)

julia> @time gif_output = GifOutput(init;
           filename="forestfire.gif", tspan=1:200, fps=25,
           minval=DEAD, maxval=BURNING,
           scheme=ColorSchemes.rainbow,
           zerocolor=RGB24(0.0),
           font=FTFont("/usr/share/fonts/truetype/Cantarell-Regular.otf")
       )
  0.010344 seconds (26 allocations: 123.905 MiB)

This would also allow to do using FreeTypeAbstraction; font = findfont("Cantarell") once,
and feed that to all font= kwargs.

Also, later autoconf could return an FTFont instead of a String, saving one findfont call.

Two questions:

  1. savefig is a bit faster (14s instead of 21s here), but there are still several autoconf calls.
    For another PR, or would you rather treat all font issues at once ?

  2. Two specific TextConfig tests are in test/image.jl.
    If more are to be added, should they all be in a new test/textconfig.pl file ?

src/outputs/textconfig.jl Outdated Show resolved Hide resolved
@rafaqz
Copy link
Member

rafaqz commented Jan 18, 2022

Thanks!!

  1. I agree we can fix this everywhere, but do as much as you feel is closely related and also doable in your time budget. Separating additional things to another PR is fine but not necessary.
  2. Yes, a text-specific test file may make things clearer.

I guess we can store an FTFont in a global Dict and check it so that it only happens once for any font name String?

Co-authored-by: Rafael Schouten <[email protected]>
@ederag
Copy link
Contributor Author

ederag commented Jan 18, 2022

I guess we can store an FTFont in a global Dict and check it so that it only happens once for any font name String?

We could, but as you said,

[...] moving Makies cache to FreeTypAbstraction seems to make sense, we shouldn't have that implemented everywhere.

Looks like how they are going to solve JuliaGraphics/FreeTypeAbstraction.jl#67, so no need here ?

@rafaqz
Copy link
Member

rafaqz commented Jan 18, 2022

Yes, it would be better, but from that conversation I guess I'm not sure if/when that will happen.

You may need to read "they" as @ederag, as Simon is pretty busy... if you have time to make those changes I'm sure people would be happy with them.

@ederag
Copy link
Contributor Author

ederag commented Jan 28, 2022

About FreeTypeAbstraction, the best course of action is unclear to me, later then.

This PR could be ready to review.

Next step would be to make autoconf() return an FTFont, but this might be breaking,
so better do that in another PR (easier to revert).
Unless you prefer to keep everything here.

@rafaqz
Copy link
Member

rafaqz commented Feb 7, 2022

Sorry I forgot about this (please bug me about things if I do). I think we can just merge this.

But autoconf is not in the api, its just an internal detail. So you can do whatever you want with it. Returning an FTFont will have the same visual outcome, but just be faster?

@rafaqz rafaqz merged commit 9961bd8 into cesaraustralia:master Feb 7, 2022
@ederag
Copy link
Contributor Author

ederag commented Feb 7, 2022

Thanks, there was no hurry, and I did not want to disturb you; you were making great contributions with the TTFX stuff 🙂

autoconf is not in the api, its just an internal detail. So you can do whatever you want with it.

Good, thanks for the go-ahead.

Returning an FTFont will have the same visual outcome, but just be faster?

Exactly. Done in #207.

@ederag ederag deleted the allow_FTFont branch February 7, 2022 22:44
@rafaqz
Copy link
Member

rafaqz commented Feb 8, 2022

You might find DynamicGridsInteract.jl ElectronOutput will load in a few seconds when all those PRs are 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.

2 participants