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

Use skrifa::NormalizedCoord to represent normalized coordinates #76

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nicoburns
Copy link
Collaborator

The motivation for this PR is to avoid this conversion boilerplate (and otherwise unnecessary Vec allocation) when integrating parley (which uses swash's NormalizedCoord) with vello (which uses skrifa's NormalizedCoord).

Once parley is updated with this change it should be possible to pass run.normalized_coords directly to draw_glyphs().normalized_coords.

This PR assumes that the bit representation of normalized coords is the same in Swash and Skrifa, and that Skrifa just has a newtype around the raw bits.

@nicoburns nicoburns requested a review from dfrg November 27, 2024 22:55
Copy link
Owner

@dfrg dfrg left a comment

Choose a reason for hiding this comment

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

This looks reasonable and makes sense. The one downside is that updating the skrifa version is now potentially a breaking change but the interop benefit is probably worth it.

@nicoburns
Copy link
Collaborator Author

The one downside is that updating the skrifa version is now potentially a breaking change but the interop benefit is probably worth it.

Hmm... true. And I guess this is also currently true of Vello atm. Perhaps it would make sense to go the other way and use i16 as the interchange type?

This type is a re-export of F2Dot14 from font-types. So I think a skrifa bump would only be breaking if it bumps font-types. But still, we could otherwise make it entirely internal, which might be preferable?

@dfrg
Copy link
Owner

dfrg commented Nov 28, 2024

The other option is that vello’s DrawGlyphs could take an iterator instead of a slice. Doesn’t avoid the conversion but doesn’t require the allocation.

@dfrg
Copy link
Owner

dfrg commented Nov 28, 2024

I believe vello also currently re-exports skrifa so an update there is a breaking change regardless. Maybe we should rethink that.

@nicoburns
Copy link
Collaborator Author

The other option is that vello’s DrawGlyphs could take an iterator instead of a slice. Doesn’t avoid the conversion but doesn’t require the allocation.

That seems sensible anyway given that it only needs an iterator. But I was also thinking about Vello's release cycle. It would also be nice if a Skrifa bump in Vello didn't require a breaking release. Really the font-types crate ought to be serving this purpose of being the stable base (with infrequent breaking changes) that other crates can build upon. But I guess it's not quite there yet (but might be eventually?)

@dfrg
Copy link
Owner

dfrg commented Nov 28, 2024

The font-types crate has less churn than the others but it hasn’t really stabilized yet. Maybe in a few months.

In the meantime, we should consider changing vello to take an iterator, maybe of a simpler type like the current one in swash.

edit: and removing everything else that might make a skrifa update a breaking change.

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