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

Make FontSystem not self-referencing and update fontdb and rustybuzz #89

Merged
merged 5 commits into from Mar 3, 2023

Conversation

geieredgar
Copy link
Contributor

This makes the std implementation of FontSystem not self-referencing, which in turn allows defining a db_mut method for mutable access to the underlying fontdb::Database (requested in #75).

The high level changes are:

  • Adds a new FontKey struct that stores the fontdb::ID and (if the swash feature is enabled) the swash key data (u32, swash::CacheKey).
    • A FontKey can be obtained via the new FontSystem::get_font_key(id).
    • FontKeys are cached instead of caching Fonts.
  • FontSystem::get_font now takes a FontKey instead of fontdb::ID and creates a Font on the fly, reusing the key data.
  • Removes FontMatches.
    • FontSystem::get_font_matches now returns a Arc<Vec<FontKey>> instead of Arc<FontMatches<'a>>.
    • FontFallbackIter borrows fontdb::Database and creates the font on the fly using Font::from_key(db, key).

@notgull
Copy link
Contributor

notgull commented Mar 1, 2023

Just a heads up: before version 0.13.0, lookups in fontdb take linear time. As part of this PR, if you're making breaking changes anyways, I would bump fontdb to 0.13.0 so that lookups take O(1) time.

@geieredgar geieredgar changed the title Make FontSystem not self-referencing Make FontSystem not self-referencing and update fontdb Mar 1, 2023
@geieredgar
Copy link
Contributor Author

Ok, I have updated the fontdb dependency. The family name displayed in error messages is now the name of the first family in fontdb::FaceInfo or post_script_name if families is empty.

src/font/mod.rs Show resolved Hide resolved
@geieredgar geieredgar changed the title Make FontSystem not self-referencing and update fontdb Make FontSystem not self-referencing and update fontdb and rustybuzz Mar 1, 2023
@geieredgar
Copy link
Contributor Author

geieredgar commented Mar 1, 2023

I fixed the duplicate ttf-parser entries by bumping rustybuzz to version 0.7. Not sure I can/should do something about the failed license requirement.

@geieredgar
Copy link
Contributor Author

I've added Zlib to the list of allowed licenses. I think there is no harm in that.

@jackpot51
Copy link
Member

@geieredgar I merged this since CI tests were passing but it appears to have drastically reduced the performance of the editor-test example, so I reverted. Please see if you can replicate this issue.

@geieredgar
Copy link
Contributor Author

geieredgar commented Mar 3, 2023

@jackpot51 Yes, I could replicate this issue. The reason is that rustybuzz::Face::from_slice takes a lot of time. I wrongly assumed it would behave similar to ttf_parser::Face::parse in terms of performance, which is intended to be cheap and therefore can be executed on the fly, but that isn't the case. I will look into trying to make rustybuzz::Face::from_slice not as expensive and then compare the performance of my approach with the old approach. Sorry for wasting your time!

@jackpot51
Copy link
Member

Thanks for looking into this. If there was a way to do what the swash Font does it would be ideal.

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

4 participants