harfbuzz / harfbuzz_rs

A fully safe Rust wrapper for the harfbuzz text shaping library.
MIT License
52 stars 21 forks source link

docs: better sample in README.md #29

Closed drahnr closed 3 years ago

drahnr commented 3 years ago

Improve the sample code to reflect more realistic usage.

manuel-rhdt commented 3 years ago

Thanks for the PR and sorry for the delay.

I am not sure I would consider the the sample code you post to be more "realistic". I think it depends whether you want to handle font scaling through harfbuzz, or rather work in font units and do the scaling yourself. Note that if you don't call font.set_scale(x, y), harfbuzz works in font unit coordinates (e.g. 1/1000 em).

Ideally, both of these usages would be described in the documentation. So maybe I'd rather have two examples in the Readme. One that shows the basic usage without font.set_scale(x, y), and a second example that demonstrates font scaling.

drahnr commented 3 years ago

In that case comment would be sufficient, i.e. a condensed version of your paragraph: // if set_scale is not called, the default units are 0.001 em

manuel-rhdt commented 3 years ago

Personally, I still think for a novice it is quite mysterious why the font scaling is multiplied by 64, just to divide again after the fact. Also you might get rounding issues if you are not careful and your font size is small. For these reasons I would prefer to keep the example as it currently is but add a new code block to show how to convert the results from font units to pixels, etc.

I imagine something like

// Harfbuzz by default returns all values in font units. To scale these units to pixels
// we first have to get the number of font units per EM.
let units_per_em = face.upem();

// A font size of say 17px means that the size of 1 EM = 17 px. Therefore our scale
// factor to go from font units to pixels is font_size / units_per_em
let font_size = 17.0; // px
let scale_factor = font_size / (units_per_em as f64);

I think usually it is easier to do scaling this way and avoid using the harfbuzz set_scale function.

drahnr commented 3 years ago

Feel free to close this then and adjust the example as needed, as long as there is a good explanation I have no preference either way.