[variable features] [mark fea writer] Instead of skipping a glyph altogether, only skip missing source layers #840
Conversation
…rs, so we can still build meaningful variable anchors
|
@anthrotype, what do you think? |
|
it makes sense, however I think _getAnchorLists should still handle the possibility that _getAnchor returns None, I see there are other places where it does return None |
|
Something like this? diff --git a/Lib/ufo2ft/featureWriters/markFeatureWriter.py b/Lib/ufo2ft/featureWriters/markFeatureWriter.py
index 9a79bb4..427b752 100644
--- a/Lib/ufo2ft/featureWriters/markFeatureWriter.py
+++ b/Lib/ufo2ft/featureWriters/markFeatureWriter.py
@@ -356,9 +356,11 @@ class MarkFeatureWriter(BaseFeatureWriter):
self.log.warning(
"duplicate anchor '%s' in glyph '%s'", anchorName, glyphName
)
- x, y = self._getAnchor(glyphName, anchorName, anchor=anchor)
- a = self.NamedAnchor(name=anchorName, x=x, y=y)
- anchorDict[anchorName] = a
+ anchorCoord = self._getAnchor(glyphName, anchorName, anchor=anchor)
+ if anchorCoord is not None:
+ x, y = anchorCoord
+ a = self.NamedAnchor(name=anchorName, x=x, y=y)
+ anchorDict[anchorName] = a
if anchorDict:
result[glyphName] = list(anchorDict.values())
return resultI can gladly add that to this PR, but I don't really have means to properly test it. |
|
actually I think the None never triggers. The way _getAnchor gets called from _getAnchorLists makes it impossible for None to be returned; in a variable context, the default source will contain a glyphName with that anchorName because caller got them from itself; and in non-variable context, the Maybe @simoncozens could also take a look, I think he wrote that code. It's kind of strange that this code was not originally written with a Adding the continue will essentially allow "sparse" anchor definitions, which aligns with the intent of the sparse glyph layers |
ideally we should add a test like the ones in tests/featureWriters/markFeatureWriter_test.py |
it's more like "crashing" -- which I agree is not ideal. I wonder why we haven't found this early, must be a very common situation. Ah.. I see! You are using whole UFOs (their default foreground layer) as sparse sources, not additional sparse layers within a |
Indeed. Which is a very natural thing to do in my context, and I think it would be nice if this were better supported. Especially in cases that involve a two-line fix :) My context: merging several projects, but not all support support the same variation axes. For example, one subproject supports opsz, and has (non-sparse) masters for that, but that leaves the other subproject's glyphs "sparse" compared to the whole system. But I still need to build its mark feature. |
|
I think the PR as it is would work fine. I've never quite understood why we use layers for sparse masters; we end up having to associate a sparse master with an arbitrary non-sparse master. It doesn't make sense to me that e.g. "wght=450" is "part" of Regular. The plan in babelfont is just to promote them to masters and deal with it. |
It comes from Glyphs.app, I think, where intermediate (sparse) layers are associated with some "master" layer. I'm in favour of better support for sparse UFO as sources. We can merge this PR as soon as we have a little test. maybe modify the TestVarfea.designspace or the source UFOs it references to add sparse UFO or make one of the existing one sparse, put some anchors and voila |
Done. The test indeed confirms the effectiveness of the fix: without it we observe the crash. Although the result of my added glyphs is not a variable mark, I think it still proves the fix is correct. |
|
(Apologies for the irrelevant fontinfo.plist change, I can roll that back if you want.) |
|
it's ok |
Instead of skipping a glyph altogether, only skip missing source layers, so we can still build meaningful variable anchors.
Situation:
Currently, I get a failure in this situation:
This is because _getAnchor() returns None for such a glyph in the
self.context.isVariablecode path.However, it is trivial to make this work by simply skipping the sources that do not contain the glyph. That's what this PR does.