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 feature for drawing title text #8

Merged
merged 3 commits into from
Aug 14, 2022

Conversation

alexheretic
Copy link
Contributor

@alexheretic alexheretic commented Aug 14, 2022

This pr provides an alternative to crossfont/"title" feature using crate ab_glyph to render title text.

This is helpful in some cases since the latter is pure rust requiring no extra dynamically linked dependencies or cmake freetype builds etc. (Related rust-windowing/winit#2373 (comment)).

ab_glyph titles use an embedded Cantarell-Regular.ttf font & hardcoded size selected to best match the current crossfont title size. I'm not sure how hard it would be to find the system default font file & size, but this seems a decent starting point.

cargo run --example simple --features crossfont

cargo run --example simple (new default feature ab_glyph)

Update: Made ab_glyph feature default. This is a breaking change.

Copy link
Owner

@PolyMeilex PolyMeilex left a comment

Choose a reason for hiding this comment

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

Codewhise everything looks good to me, just a minor comment about defaults, I want ab_glyph to be the goto version, and corssfont one should only be for people that accually know why they want to use it. That is obviously a braking change, but we are kinda braking anyway by removing the default feature.

Also a small question, assuming that I figure out a way to replace fontconfig will ab_glypg be able to handle most font types that people may use with freetype. Just trying to figure out if it's worth the effort.

Cargo.toml Outdated Show resolved Hide resolved
@alexheretic
Copy link
Contributor Author

Yeah defaults are up to you, I went with non-default to be minimally breaking. I can change that. Maybe we can just lose the title feature and default to ab_glyph optional dependency?

I think ab_glyph should have good compatibility thanks to the work in ttf-parser. The current pr just uses a single embedded font & size. If we knew where the desired/system font ttf/otf file was and the desired size that should work fine too, we just need to look it up somehow.

I considered looking into that, but even if we had that it's probably worth having a reasonable fallback too.

@alexheretic
Copy link
Contributor Author

I've switched the new renderer to be default & updated the readme with how to use crossfont.

@PolyMeilex
Copy link
Owner

PolyMeilex commented Aug 14, 2022

Maybe we can just lose the title feature and default to ab_glyph optional dependency?

Sure, sounds good to me.

I considered looking into that, but even if we had that it's probably worth having a reasonable fallback too.

If we want to replace fontconfig we would most likely have to do something similar to https://github.com/fschutt/rust-fontconfig (or just use it if possible) but that's a whole another issue that should be done separately. For now I'm fine with embedded font, still better that people disabling title rendering all together just to get it to build without cmake.

And once again, thank you for working on this ❤️

@alexheretic
Copy link
Contributor Author

I agree, in which case this is ready to merge 👍

@PolyMeilex PolyMeilex merged commit 59720f0 into PolyMeilex:master Aug 14, 2022
@alexheretic alexheretic deleted the ab-glyph branch August 14, 2022 16:29
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