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.

Sign up for GitHub

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

Jump to bottom

Remove skrifa from vello's public API #747

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Remove skrifa from vello's public API #747

wants to merge 4 commits into from

Conversation

nicoburns
Copy link
Contributor

The motivation for this is two-fold:

  • To align vello's representation of "normalized coordinates" with that of swash/parley (avoiding a boilerplatey conversion when integrating parley with vello)
  • To make skrifa version bumps in vello non-breaking changes.

@nicoburns nicoburns added the dependencies Pull requests that update a dependency file label Nov 28, 2024
nicoburns
nicoburns commented Nov 28, 2024
View reviewed changes
vello/src/scene.rs
@@ -400,7 +400,7 @@ impl<'a> DrawGlyphs<'a> {
}

/// Sets the normalized design space coordinates for a variable font instance.
pub fn normalized_coords(mut self, coords: &[NormalizedCoord]) -> Self {
pub fn normalized_coords(mut self, coords: impl Iterator<Item = i16>) -> Self {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could require ExactSizeIterator here if we want to.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don’t think it would hurt to have a pub type NormalizedCoord = i16 in vello_encoding

Copy link
Member

Choose a reason for hiding this comment

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

What does ExactSizeIterator win us?

Copy link
Member

Choose a reason for hiding this comment

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

I think we definitely should document this slightly better, even if just to say "this will be provided by your text layout implementation". The current format is quite unactionable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does ExactSizeIterator win us?

Potentially slightly cheaper allocation. But I suspect size_hint will work even without ExactSizeIterator, and that even if it doesn't the cost will be small.

@dfrg
Copy link
Collaborator

dfrg commented Nov 28, 2024

Note this type is still visible from vello_encoding:

vello/vello_encoding/src/encoding.rs

Line 548 in e6ead09

pub normalized_coords: Vec<NormalizedCoord>,

This is a bit more tricky because skrifa actually wants a &[NormalizedCoord] type. We can either keep an internal buffer in the resolver and do a copy or just use bytemuck to cast the slice (font_types::F2Dot14 impls the necessary traits)

DJMcNab
DJMcNab reviewed Nov 28, 2024
View reviewed changes
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

Looks good. This also needs a CHANGELOG entry

examples/scenes/src/simple_text.rs Show resolved Hide resolved
examples/scenes/src/simple_text.rs Outdated
@@ -210,7 +217,7 @@ impl SimpleText {
}

fn to_font_ref(font: &Font) -> Option<FontRef<'_>> {
use vello::skrifa::raw::FileRef;
use skrifa::raw::FileRef;
Copy link
Member

@DJMcNab DJMcNab Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is there much advantage to this import being here rather than at the top of the file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. That was where it already was, but I've moved it to the top.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, apologies. I probably didn't originally notice that this was only a change and not an addition.

vello/src/scene.rs
@@ -400,7 +400,7 @@ impl<'a> DrawGlyphs<'a> {
}

/// Sets the normalized design space coordinates for a variable font instance.
pub fn normalized_coords(mut self, coords: &[NormalizedCoord]) -> Self {
pub fn normalized_coords(mut self, coords: impl Iterator<Item = i16>) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

What does ExactSizeIterator win us?

vello/src/scene.rs
@@ -400,7 +400,7 @@ impl<'a> DrawGlyphs<'a> {
}

/// Sets the normalized design space coordinates for a variable font instance.
pub fn normalized_coords(mut self, coords: &[NormalizedCoord]) -> Self {
pub fn normalized_coords(mut self, coords: impl Iterator<Item = i16>) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

I think we definitely should document this slightly better, even if just to say "this will be provided by your text layout implementation". The current format is quite unactionable.

@waywardmonkeys
Copy link
Contributor

@nicoburns Can we wrap this one up and land it? I'd like to make sure this is in our upcoming release.

@nicoburns
Copy link
Contributor Author

How much do we still care about this being public in vello_encoding? Seems to me that it's probably less important to do semver compatible releases there? It's easy enough to change everything to i16, but it involves changing a lot of the internal uses of NormalizedCoord into i16 which seems like somewhat of a code quality regression from a readability / type safety point of view.

@nicoburns
Add changelog entry
e847f5f 
Signed-off-by: Nico Burns <[email protected]>
@nicoburns
cargo fmt
e152003 
Signed-off-by: Nico Burns <[email protected]>
@DJMcNab
Copy link
Member

DJMcNab commented Dec 10, 2024

I don't hate making vello_encoding intentionally not follow semver, but we'd need to do two things:

  1. Document that being the case
  2. Use an = dependency specification on it from Vello.

@nicoburns
Copy link
Contributor Author

nicoburns commented Dec 10, 2024 •
edited
Loading

I don't hate making vello_encoding intentionally not follow semver, but we'd need to do two things:

  1. Document that being the case
  2. Use an = dependency specification on it from Vello.

My suggestion would be for vello_encoding to continue to follow semver, but to aggressively bump it's version number beyond that of the main vello crate's (as required). I feel like "you're using the low-level API, so you have to deal with breaking changes more often" is kinda reasonable, especially if the "breaking change" is just a dep bump. Also, I believe most users of vello_encoding are unlikely to have large crates.io dep trees. So conflicting versions is likely to be less of an issue.

@DJMcNab
Copy link
Member

DJMcNab commented Dec 10, 2024

Looking again, I'm not sure that would be practical; Vello itself exposes vello_encoding as public API. I don't know the right answer here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DJMcNab DJMcNab DJMcNab left review comments

@dfrg dfrg dfrg left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nicoburns @dfrg @waywardmonkeys @DJMcNab