fix `text_size`

[cospectrum]

First I tried h = h.max(bb.min.y + bb.height());. But then I think I found a better solution. Looks good when using English and Japanese text. And some useful distance at the bottom of the text. You can try on my branch. Relates: https://github.com/image-rs/imageproc/issues/687

[cospectrum]

I think clippy should be optional because we use the nightly version. But there are no "allowed to fail" jobs on github-actions, thus, we will live with the red status

[cospectrum]

It's not that bad. I see that it discovered unnecessary lifetimes and stuff. We just have to fix it every couple of months.

[cospectrum]

Maybe we can add more versions to clippy matrix

matrix:
    toolchain: [nightly]

[cospectrum]

Maybe we can add more versions to clippy matrix

matrix:
    toolchain: [nightly]

No, stable clippy doesn't understand test code with nightly features

[cospectrum]

I left text rendering unchanged, didn't find any fatal errors.

rect.height() == font.height().

[theotherphil]

Thanks!

it would be nice to extend the text regression test to include the issue this is fixing: https://github.com/image-rs/imageproc/blob/54573dabc28e0fdd8f978843e253d06bc763cf14/tests/regression.rs#L810

[cospectrum]

Ofc. I will write proptests to ensure that text is always rendered inside this rectangle.

[cospectrum]

And now I have found a counterexample for the X position in draw_text itself (first j). DejaVuSans.ttf

let img = GrayImage::new(600, 150);
let x = 62;
let y = 21;
let scale = 78.0932;
let text = "jnenuoltqjylzcbvcc";

let (text_w, text_h) = text_size(scale, &font, text);
let rect = Rect::at(x, y).of_size(text_w, text_h);

[cospectrum]

Maybe the origin of the first glyph? its (0, font.ascent()). Or it can be due to usage of bbox starting positions, they can be negative actually

[cospectrum]

I tried to fix it, but it requires replacing the text.png sample from the regression tests.

[cospectrum]

proptest passed for english text with numbers

[theotherphil]

The test image can be regenerated by running

REGENERATE=1 cargo test

[cospectrum]

Are you familiar with OpenType glyphs? I didn't plan to change draw_text at all. But now I'm shifting all glyphs to the right by abs(h_side_bearing) of the first glyph, because some glyphs go beyond of Rect.at(x, y), and it's not just a floating point error.

[cospectrum]

I replaced text.png with mine

[cospectrum]

I think that despite moving to the right, all relative distances between glyphs will remain the same as in the previous implementation.

[cospectrum]

For example, in the previous implementation of draw_text, the j with (x, y) = (0, 0) will go beyond the image. Thats what I tried to fix

[cospectrum]

We can go the other way. We can keep the starting point at 0, that is, no change in the draw_text implementation. Instead, we will change my new tests so that the "checked" rectangle itself takes into account the negative horizontal bearing. What do you think? @theotherphil

[cospectrum]

We can also add the horizontal bearing to the origin only if it is negative. Proptest will pass.

let first_h_bearing = font.h_side_bearing(first_glyph);
let mut w = if first_h_bearing < 0.0 {
    first_h_bearing.abs()
} else {
    0.0
};

[cospectrum]

I have no preferences here.

[theotherphil]

I’m not familiar with OpenType.

Any of these options seems fine - let’s go with relaxing the test to allow for negative side bearings if you have no preference.

[cospectrum]

Sure, one moment

[cospectrum]

I added +1 because otherwise rect.contains will fail. +1 in proptest looked like cheating

[theotherphil]

Thanks a lot!