googlefonts / ufo2ft

A bridge from UFOs to FontTools objects (and therefore, OTFs and TTFs).
MIT License
152 stars 43 forks source link

Variable Feature code path optimization does not take discrete axes into account #865

Closed justvanrossum closed 2 months ago

justvanrossum commented 3 months ago

This is about the _featuresCompatible() function: it determines whether the features in a designspace system are compatible, so that kern/mark/mkmk etc can be generated via Variable Feature syntax.

I ran into a situation where this optimization is crucial, because this code path handles sparse kerning much better: without this, fontmake would fail.

Now, in a system with discrete axes, we may expect features to be different at different axis positions. _featuresCompatible() inspects the UFO features globally, so will fall back to non-variable-feature syntax kern/mark/mkmk/etc compilation, even though the individually split systems are compatible.

So, in an ideal world, _featuresCompatible() would look at the individual sub-spaces, and not at the whole designspace.

justvanrossum commented 3 months ago

@gaetanbaehr

anthrotype commented 3 months ago

thanks for this. I think we already use splitInterpolable function from designspaceLib when compiling the features variably, so we just need to check for their compatibility in the same way.

Does this work for you? Can you send a PR and also maybe provide a minimal test font?

diff --git a/Lib/ufo2ft/_compilers/baseCompiler.py b/Lib/ufo2ft/_compilers/baseCompiler.py
index d1b6c1fd..2e877a63 100644
--- a/Lib/ufo2ft/_compilers/baseCompiler.py
+++ b/Lib/ufo2ft/_compilers/baseCompiler.py
@@ -326,8 +326,8 @@ class BaseInterpolatableCompiler(BaseCompiler):

         # If the feature files are compatible between the sources, we can save
         # time by building a variable feature file right at the end.
-        can_optimize_features = self.variableFeatures and _featuresCompatible(
-            designSpaceDoc
+        can_optimize_features = self.variableFeatures and all(
+            _featuresCompatible(doc) for doc in interpolableSubDocs
         )
         if can_optimize_features:
             self.logger.info("Features are compatible across masters; building later")
justvanrossum commented 3 months ago

Awesome, that looks like a reasonable solution. I'll come back to this later.

GaetanBaehr commented 3 months ago

I tried locally this fix in my venv, it's working perfectly! thanks a lot

justvanrossum commented 3 months ago

I’ll make a pr soon, thanks.