linebender / norad

Rust crate for working with Unified Font Object files
Apache License 2.0
44 stars 12 forks source link

Designspaces: support deserialising empty condition sets #327

Closed RickyDaMa closed 9 months ago

RickyDaMa commented 9 months ago

Error:

failed to deserialize designspace: missing field condition

Debug representation:

DeError(                            
    Custom(                         
        "missing field `condition`",
    ),                              
)
Example reproducer: modded Mutator Sans designspace

```xml ```

Yes the fields could just be removed, but I also don't think this should be egregious enough to fail deserialisation entirely

cmyr commented 9 months ago

an empty conditionset is invalid, per the spec (it "Contains one or more elements."). We could add custom logic to allow parsing it if empty and then discard it, if this is particularly common in the wild.

Is this getting written by designspaceLib? if so we should also open an issue there, as this looks like a spec violation.

rsheeter commented 9 months ago

If it turns out desigbspacelib does this then it might be we should update the spec to match the implementation, check with Cosimo.

On Tue, Oct 10, 2023, 11:48 AM Colin Rofls @.***> wrote:

an empty conditionset is invalid, per the spec (it "Contains one or more elements."). We could add custom logic to allow parsing it if empty and then discard it, if this is particularly common in the wild.

Is this getting written by designspaceLib? if so we should also open an issue there, as this looks like a spec violation.

— Reply to this email directly, view it on GitHub https://github.com/linebender/norad/issues/327#issuecomment-1755726709, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABRKXAHCL3QUF5NZVPUSYCTX6VU4ZAVCNFSM6AAAAAA5UDNJA2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONJVG4ZDMNZQHE . You are receiving this because you are subscribed to this thread.Message ID: @.***>

anthrotype commented 9 months ago

well, it is valid to have an empty ConditionSet in the OpenType spec:

https://learn.microsoft.com/en-us/typography/opentype/spec/chapter2#conditionset-table

If a given condition set contains no conditions, then it matches all contexts, and the associated feature table substitution is always applied

it is also true that the designspace spec says "one or more": https://fonttools.readthedocs.io/en/latest/designspaceLib/xml.html#conditionset-element

I think discarding empty conditionset is not correct in light of "matches all contexts" paragraph above, it's the opposite of no-op, it's actually always on.

@RickyDaMa do you have a concrete use case for this?

I haven't tried what fonttools varLib does with it, if it actually builds the empty conditionset or skips it, but I'd expect it should do the former.

belluzj commented 9 months ago

The implementation of designspace v5 to split one big designspace into smaller ones may emit empty conditionsets when a given rule should always be active in the sub-designspace that we're emitting. See: https://github.com/fonttools/fonttools/blob/98242634c45cf4ca9f8aff6fd059de9dcf734471/Lib/fontTools/designspaceLib/split.py#L402

image

anthrotype commented 9 months ago

cool thanks, makes sense. So I imagine that building off that will generate a VF with a FeatureVariationRecord containing an empty (always-on) ConditionSet. If somebody can give that a try and confirm that's the case, we can update the DesignSpace spec to match this. And norad as well.

madig commented 9 months ago

VarLib makes an empty condition set.