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

First step toward marks #492

Merged
merged 6 commits into from
Oct 31, 2023
Merged

First step toward marks #492

merged 6 commits into from
Oct 31, 2023

Conversation

rsheeter
Copy link
Contributor

@rsheeter rsheeter commented Oct 19, 2023 •
edited
Loading

Starting to work toward marks based on discussion with @anthrotype. Likely to be step one in a series of PRs.

Helps with #308.

Builds on @cmyr's work to avoid generating fea strings in #496. Rebased to sit on top of fea-rs merge into fontc and the feature writer api (#517).

Includes API sketch from cmyr/fea-rs#243, updated per cmyr/fea-rs#243 (comment).

fontbe/src/features.rs is the heart of it.

@rsheeter rsheeter force-pushed the back_when_mark_was_marky_mark branch 4 times, most recently from 0c1090f to 08ed079 Compare October 23, 2023 03:16
@rsheeter rsheeter changed the base branch from main to feature-writer October 23, 2023 03:17
@rsheeter rsheeter force-pushed the back_when_mark_was_marky_mark branch 2 times, most recently from ade3346 to f5ac1ba Compare October 24, 2023 04:54
@rsheeter rsheeter force-pushed the back_when_mark_was_marky_mark branch 3 times, most recently from 4134a80 to af0348f Compare October 26, 2023 03:33
@rsheeter rsheeter changed the base branch from feature-writer to fea-api October 26, 2023 03:37
@rsheeter rsheeter mentioned this pull request Oct 26, 2023
Closed
@rsheeter rsheeter force-pushed the back_when_mark_was_marky_mark branch from e8bba33 to 211b69b Compare October 26, 2023 04:00
Base automatically changed from fea-api to main October 26, 2023 16:37
@rsheeter rsheeter force-pushed the back_when_mark_was_marky_mark branch 4 times, most recently from 4c71715 to 6078f3a Compare October 27, 2023 21:53
cmyr
cmyr reviewed Oct 30, 2023
View reviewed changes
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

some observations inline but looks like we're very close!

fontbe/src/features.rs Outdated Show resolved Hide resolved
fontbe/src/features.rs Outdated Show resolved Hide resolved
fontbe/src/features.rs Outdated Show resolved Hide resolved
fontbe/src/features.rs Outdated Show resolved Hide resolved
fontbe/src/features.rs Outdated Show resolved Hide resolved
@cmyr cmyr mentioned this pull request Oct 30, 2023
Merged
@rsheeter rsheeter force-pushed the back_when_mark_was_marky_mark branch from e722100 to f70dab4 Compare October 30, 2023 23:53
@rsheeter rsheeter force-pushed the back_when_mark_was_marky_mark branch from f70dab4 to c05ebfc Compare October 30, 2023 23:56
@rsheeter rsheeter force-pushed the back_when_mark_was_marky_mark branch from c05ebfc to 4d6f960 Compare October 31, 2023 00:08
@rsheeter rsheeter mentioned this pull request Oct 31, 2023
Open
@rsheeter rsheeter changed the title [WIP] Marks First step toward marks Oct 31, 2023
@rsheeter rsheeter marked this pull request as ready for review October 31, 2023 00:17
@rsheeter rsheeter force-pushed the back_when_mark_was_marky_mark branch from 083e7ba to 01ad09c Compare October 31, 2023 00:50
@anthrotype anthrotype mentioned this pull request Oct 31, 2023
Open
anthrotype
anthrotype reviewed Oct 31, 2023
View reviewed changes
Copy link
Member

@anthrotype anthrotype left a comment

Choose a reason for hiding this comment

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

nice to see this is making progress! left some comment

fontir/src/ir.rs Outdated Show resolved Hide resolved
fontc/src/lib.rs Show resolved Hide resolved
fea-rs/src/compile/variations.rs Show resolved Hide resolved
fontbe/src/features.rs Outdated Show resolved Hide resolved
fontbe/src/features.rs Outdated Show resolved Hide resolved
fontbe/src/features.rs Show resolved Hide resolved
@cmyr cmyr mentioned this pull request Oct 31, 2023
Merged
cmyr
cmyr approved these changes Oct 31, 2023
View reviewed changes
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

Some notes inline but I think we should get this in and iterate from here!

fea-rs/src/compile/variations.rs Show resolved Hide resolved
fontbe/src/error.rs Show resolved Hide resolved
fontc/src/lib.rs Show resolved Hide resolved
fontc/src/lib.rs Show resolved Hide resolved
fontir/src/ir.rs Outdated Show resolved Hide resolved
fontbe/src/features.rs
let right = glyph_classes.get(right).unwrap().clone();
let left = glyph_classes
.get(left)
.ok_or_else(|| Error::MissingGlyphId(left.clone()))?
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we could pre-validate the kerning info at an earlier point in the pipeline?

@cmyr cmyr mentioned this pull request Oct 31, 2023
Open
This was referenced Oct 31, 2023
Closed
Closed
rsheeter and others added 2 commits October 31, 2023 12:52
@rsheeter rsheeter added this pull request to the merge queue Oct 31, 2023
Merged via the queue into main with commit 867558e Oct 31, 2023
9 checks passed
@rsheeter rsheeter deleted the back_when_mark_was_marky_mark branch October 31, 2023 20:02
@simoncozens simoncozens mentioned this pull request Nov 8, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anthrotype anthrotype anthrotype left review comments

@cmyr cmyr cmyr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rsheeter @cmyr @anthrotype