Conversation
Hoolean
left a comment
There was a problem hiding this comment.
Thanks for tackling, would love to have something like this in the shared Rust toolbox
Retains re-export of Kerning type alias
Only iterate through groups once
9a61a15 to
d259411
Compare
cmyr
left a comment
There was a problem hiding this comment.
A couple notes in line, and the biggest point I'd make is already made in the earlier discussion: I think it would be nice if this was simplified to some kind of opaque KerningResolver type that ideally borrows data from the font. I'd also prefer to not have the two APIs, and just always compute the reverse map, unless there is some non-theoretical performance concern you're addressing.
Overall though, I'm happy with the idea of something like this being merged :)
|
Hi @cmyr, I've updated the PR based on some of your comments. I've removed the slow version of the method, and updated the documentation of the remaining method. I've made
Nothing stops the user from dropping their
|
Remove slow version of method Rename ReverseGroupsLookup to ReverseGroups Make ReverseGroups hold references so its lifetime depends on the Font, also preventing edits without dropping it impl Borrow<str> for &Name
75cdb73 to
fdf191e
Compare
src/font.rs
Outdated
| /// | ||
| /// This method is requires a [`ReverseGroups`], which can be obtained from | ||
| /// [`Font::get_reverse_groups_lookup`]. | ||
| pub fn kerning_lookup( |
There was a problem hiding this comment.
okay sorry for all the back-and-forth, but what I had in mind was an API that looks something like,
struct LookupKerning<'a> {
first_groups: HashMap<Name, Name>,
second_groups: HashMap<Name, Name>,
kerning: &'a Kerning,
}
impl LookupKerning<'_> {
fn get(&self, first: &str, second: &str) -> Option<f64> {
todo!("basically the kerning_lookup method in this PR")
impl Font {
/// returns a thing you can lookup kerns in
fn lookup_kerning<'a>(&'a self) -> LookupKerning<'a> {
todo!("build the thing")
}
}
/// and then it works like,
let my_font: Font = load_my_font();
let kemings = my_font.lookup_kerning();
let a_value = kemings.lookup("A", "V");
... does that make sense? Is there a reason to prefer the version you've provided here?There was a problem hiding this comment.
Back & forth is fine! Please don't feel bad, I actively enjoy discussing and refactoring this stuff. I like your/Harry's idea more too, I'll shift my implementation to match it 👍
Rename ReverseGroups to KerningResolver KerningResolver holds a reference to the kerning Move kerning lookup method to KerningResolver Go back to using owned Names within KerningResolver
|
Thanks for the merge! 🚀 |
Every so often I get surprised that this isn't already part of norad, so I threw something together.
I've made a slow version for people only doing a couple kerning lookups and don't want the overhead of generating the reverse group lookup, and then there's also a
ReverseGroupsLookupstruct now which does exactly what it says on the tin and can get used to accelerate kerning lookups.There is currently no check that you are using the
ReverseGroupsLookupthat corresponds to theFontyou're calling the lookup on, but I'll document that caveat when I throw together some documentation ("garbage in, garbage out" or words to that effect). I could tie the two together with ✨ fancy ✨ API/types if we wanted.I also made constants for the kerning group prefixes and did a quick & dirty replace throughout the codebase, so I can let autocomplete do the work instead of worrying that I've typo'd. Bikeshedding encouraged, I don't like how long their names are.