Skip to content

Add VVAR for variable fonts that support vertical#1666

Merged
rsheeter merged 3 commits into
mainfrom
vvar1
Sep 30, 2025
Merged

Add VVAR for variable fonts that support vertical#1666
rsheeter merged 3 commits into
mainfrom
vvar1

Conversation

@rsheeter

@rsheeter rsheeter commented Sep 29, 2025

Copy link
Copy Markdown
Contributor

@cmyr pointed out we have a bunch of VVAR fontmake only diffs in crater, first swing at adding it.

Makes at least the following identical:

  • python3 resources/scripts/ttx_diff.py 'https://github.com/notofonts/ethiopic?1cc6933c58#sources/NotoSerifEthiopic.glyphs'
  • python3 resources/scripts/ttx_diff.py 'https://github.com/notofonts/tamil?626575fa0c#sources/NotoSerifTamil.glyphs'

Hopefully helps with others whose diff is (VVAR) as well.

Mostly just boilerplate and shuffling things to not be HVAR-only. Note that resources/testdata/HVVAR/SingleModel_Direct/SingleModelDirect-Regular.ufo/fontinfo.plist now specifies the magic keys to trigger build vertical.

@rsheeter rsheeter force-pushed the vvar1 branch 4 times, most recently from 34427b7 to dd1f927 Compare September 30, 2025 00:37
@rsheeter rsheeter changed the title [WIP] Create VVAR Add VVAR for variable fonts that support vertical Sep 30, 2025
@anthrotype

Copy link
Copy Markdown
Member

@rsheeter did you see this #1489 ?

@anthrotype

Copy link
Copy Markdown
Member

one thing #1489 has which this is missing is gvar vertical phantom points. We can add those as follow up

@anthrotype

Copy link
Copy Markdown
Member

btw, both NotoSerifEthiopic.glyphs and NotoSerifTamil.glyphs may now be identical in both fontc and fontmake, but they actually produce a no-op VVAR with empty VarData items, so they may not actually be the best candidates for testing:

  <VVAR>
   <Version value="0x00010000"/>
    <VarStore Format="1">
      <Format value="1"/>
      <VarRegionList>
        <!-- RegionAxisCount=2 -->
        <!-- RegionCount=0 -->
      </VarRegionList>
      <!-- VarDataCount=1 -->
      <VarData index="0">
        <!-- ItemCount=222 -->
        <NumShorts value="0"/>
        <!-- VarRegionCount=0 -->
        <Item index="0" value="[]"/>
        <Item index="1" value="[]"/>
        <Item index="2" value="[]"/>
        <Item index="3" value="[]"/>
        <!-- empty [] all the way down to the end -->

I wonder why do we even bother compiling such a VVAR...

@anthrotype

Copy link
Copy Markdown
Member

oh right, so in Glyphs.app if any glyph has vertWidth or vertOrigin set, it's deemed "vertical". Those sources I mentioned above are probably like that and get VVAR because some glyphs by accident has one of those:

https://github.com/googlefonts/glyphsLib/blob/c4db6b981d577f456d64ebe9993818770e170454/Lib/glyphsLib/builder/builders.py#L197-L198

Comment thread fontbe/src/vvar.rs Outdated
Comment thread fontbe/src/vvar.rs Outdated
Comment thread fontbe/src/error.rs
#[error("Inconsistent palette lengths observed: {0:?}")]
InconsistentPaletteLength(Vec<usize>),
#[error("No metrics instance for '{0:?}'")]
NoGlobalMetricsInstance(NormalizedLocation),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

actually, the correct thing to do is not to error when we don't have a GlobalMetricsInstance at the same location as a given glyph instance, but we should interpolate one! #1518 was added by @Hoolean exactly for this purpose.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Should that just be how accessing metrics works? Is there a case where we don't want that?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you'll actually never hit this NoGlobalMetricsInstance error because when you call GlobalMetrics::at below inside AdvanceDeltas::new, new GlobalMetricsInstances will be interpolated automatically if missing:

        let metrics = glyph_locations
            .into_iter()
            .map(|loc| {
                let loc = loc.subset_axes(&axes);
                let metrics = global_metrics.at(&loc);
                (loc, metrics)
            })
            .collect();

we can probably get rid of it altogether and simply assert/unwrap

@anthrotype anthrotype Sep 30, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a case where we don't want that?

no, we do want to interpolate all the time

@rsheeter

Copy link
Copy Markdown
Contributor Author

@rsheeter did you see this #1489 ?

Nope, just noticed in conversation with Colin and filled in some of the blanks! If that is better happy to land that and close this.

@anthrotype

Copy link
Copy Markdown
Member

If that is better happy to land that and close this.

I prefer this one because it extracts the common code in a shared metric_variations.rs and has tests. But we can incorporate the missing bits from that one here. I'm already working on that actually

Comment thread fontc/src/lib.rs Outdated

@anthrotype anthrotype left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i think we can land this first, and then follow up with the gvar vertical phantom points in a subsequent PR

@rsheeter rsheeter added this pull request to the merge queue Sep 30, 2025
Merged via the queue into main with commit 3829e52 Sep 30, 2025
12 checks passed
@rsheeter rsheeter deleted the vvar1 branch September 30, 2025 15:38
anthrotype pushed a commit that referenced this pull request Sep 30, 2025
anthrotype pushed a commit that referenced this pull request Sep 30, 2025
anthrotype pushed a commit that referenced this pull request Sep 30, 2025
anthrotype pushed a commit that referenced this pull request Sep 30, 2025
anthrotype pushed a commit that referenced this pull request Oct 4, 2025
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