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

Add ab_glyph font backend #311

Merged
merged 10 commits into from
Sep 8, 2022

Conversation

Monadic-Cat
Copy link
Contributor

I've been using plotters in a situation where caring about system fonts is unimportant, and having font-kit in my dependency tree has been causing me unnecessary problems. This PR introduces a feature gated font rendering backend based on https://github.com/alexheretic/ab-glyph, which doesn't have those problems for me.

I am willing to restructure or completely rewrite this code as necessary to get some form of this feature merged- I can't say I'm certain this is the best API for it.
I can also, and probably should also, add documentation and tests for it. Lemme know what you think is best and I'll do it.

@38
Copy link
Member

38 commented Jun 26, 2022

Hi there, sorry for a long delay. This is very helpful, and I just rebased the code. Just wondering if you are still interested in working on this PR, if yes I can send a PR to you for the rebase, otherwise I am going to work on my branch to finish this.

Please check https://github.com/plotters-rs/plotters/tree/ab_glyph_font_backend for the rebase

Thanks for the contribution and please don't mind the delay!

@Monadic-Cat
Copy link
Contributor Author

Yes! I'm absolutely still interested in working on this.

@dvdsk
Copy link

dvdsk commented Jul 1, 2022

Thanks a lot for working on this! I am using plotters in my home automation system which runs on a Pi. Without font-kit cross-compiling will be a breeze!

@Monadic-Cat
Copy link
Contributor Author

Monadic-Cat commented Aug 2, 2022

I just did a clean interactive rebase against master, after staring at the changes and your own rebased branch.
Gonna do some testing to make sure it still works, then the next thing to do will be redesigning the API.
There's some internal stuff that definitely needs fixing, as this code relies on unwinding to break out of a callback on failure.

@Zoxc
Copy link
Contributor

Zoxc commented Aug 21, 2022

I have a branch here which fixes the vertical alignment of glyphs.

@Monadic-Cat
Copy link
Contributor Author

I have a branch here which fixes the vertical alignment of glyphs.

Cool. I've tested it in my case and it looks like it works, so I'll go ahead and pull your branch here.

plotters/src/style/font/mod.rs Fixed Show fixed Hide fixed
plotters/src/style/font/ab_glyph.rs Fixed Show fixed Hide fixed
plotters/src/style/font/ab_glyph.rs Fixed Show fixed Hide fixed
plotters/src/style/font/mod.rs Fixed Show fixed Hide fixed
@Monadic-Cat
Copy link
Contributor Author

I gave up on trying to refactor things to avoid the panic reliance, and just added a buffer instead. As of now, this branch is functional enough to merge. It works where I've been using it (just as it has for... a while).
All that's left to do is fix the tests I guess I broke, and maybe bikeshed the register_fonts API if anyone cares to.

@Monadic-Cat
Copy link
Contributor Author

I've now added support for passing a FontStyle to register_font, and I believe CI should pass this now.

@Monadic-Cat
Copy link
Contributor Author

I've renamed the ab_glyph_ feature to ab_glyph, using the dep: syntax available in Cargo.toml since Rust 1.60 (which postdates this PR being opened, lol).

plotters/src/style/font/ab_glyph.rs Fixed Show fixed Hide fixed
plotters/src/style/font/ab_glyph.rs Fixed Show fixed Hide fixed
plotters/src/style/font/ab_glyph.rs Fixed Show fixed Hide fixed
plotters/src/style/font/ab_glyph.rs Fixed Show fixed Hide fixed
plotters/src/style/font/ab_glyph.rs Fixed Show fixed Hide fixed
@Monadic-Cat
Copy link
Contributor Author

Woops, those Clippy warnings tipped me off to my having forgotten to do the ab_glyph_ -> ab_glyph feature rename in one file.
I had also forgotten I'd git stashed the changes to the project I depend on this in so it wasn't using a path dependency for local testing. Fixed now.

Copy link
Member

@AaronErhardt AaronErhardt left a comment

Choose a reason for hiding this comment

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

It'd be great if all clippy warnings were fixed, but for me everything looks good. Maybe @38 can have a look too.

@38
Copy link
Member

38 commented Sep 8, 2022

I just tried this out it works perfect on my side. And this makes us away from libfontconfig dependency which has frequently asked by many people.

It would be nice if you have the clippy warning fixed. Besides that everything looks good to me.

Thanks for the awesome work!

@38 38 self-requested a review September 8, 2022 11:53
@AaronErhardt
Copy link
Member

Thanks!

@AaronErhardt AaronErhardt merged commit a4dab40 into plotters-rs:master Sep 8, 2022
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