Skip to content

Glyphs smart components#1689

Merged
cmyr merged 5 commits intomainfrom
glyphs-smart-components
Oct 14, 2025
Merged

Glyphs smart components#1689
cmyr merged 5 commits intomainfrom
glyphs-smart-components

Conversation

@cmyr
Copy link
Member

@cmyr cmyr commented Oct 9, 2025

This is a checkpoint, and only glyphs3 sources are supported. (I believe that smart components might also exist in glyphs2 sources)


pub fn has_non_zero(&self, tag: Tag) -> bool {
self.get(tag).unwrap_or_default().to_f64() != 0.0
}
Copy link
Contributor

Choose a reason for hiding this comment

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

New behavior seems testable but no new tests

"expected only 1 or 2 in partSelection, found '{other}'"
))),
})
.collect::<Result<_, _>>()?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it valid to have 2 entries that both have the same SmartComponentValue, e.g. 2 Min's?

Copy link
Member Author

Choose a reason for hiding this comment

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

a given layer could be at the min of two different axes, if that's what you mean?

_ => layer.shapes.push(shape),
};
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels more gross than it perhaps has to be?

Some(335)
);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we test some defective smart components?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've ported the tests from fonttools, although that doesn't provide a ton of coverage of malformed inputs. It doesn't seem like this stuff is really used much in the wild after all, so I'm inclined to just get this in as a checkpoint, and if we run into problems we can flesh it out more.

*range.start()
} else {
*range.end()
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the assumption that there are exactly two and they must be min and max? We don't seem to actually check this?

Copy link
Contributor

Choose a reason for hiding this comment

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

The python refs are much appreciated but ... this is fairly hard to follow. Lots of noise for iters and maps and such that make it hard to see the logic. Maybe moving some of that into helper methods and/or adding comments might help?

.collect()
}

//https://github.com/fonttools/fonttools/blob/03a3c8ed9e/Lib/fontTools/varLib/models.py#L47
Copy link
Contributor

Choose a reason for hiding this comment

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

The citation makes me wonder if this belongs somewhere in the var model code?

Copy link
Contributor

@rsheeter rsheeter left a comment

Choose a reason for hiding this comment

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

LGTM subject to addressing comments where feasible

@cmyr cmyr force-pushed the glyphs-smart-components branch from 31e91fe to b088ae2 Compare October 10, 2025 19:39
cmyr added 2 commits October 14, 2025 12:37
This is a checkpoint of an initial basic implementation, based on
glyphsLib.
@cmyr cmyr force-pushed the glyphs-smart-components branch from b088ae2 to a611cca Compare October 14, 2025 16:39
- clarify some names and docs
- start porting fonttools test cases
@cmyr cmyr force-pushed the glyphs-smart-components branch from 67f4f18 to 7a4ac82 Compare October 14, 2025 18:12
@cmyr cmyr force-pushed the glyphs-smart-components branch from 7a4ac82 to 7f3de32 Compare October 14, 2025 18:49
@cmyr cmyr added this pull request to the merge queue Oct 14, 2025
Merged via the queue into main with commit 87a7533 Oct 14, 2025
12 checks passed
@cmyr cmyr deleted the glyphs-smart-components branch October 14, 2025 20:12
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