Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test failure with SDL_ttf 2.0.18: text is 1px narrower than expected #212

Closed
smcv opened this issue Jan 13, 2022 · 3 comments · Fixed by #213
Closed

test failure with SDL_ttf 2.0.18: text is 1px narrower than expected #212

smcv opened this issue Jan 13, 2022 · 3 comments · Fixed by #213

Comments

@smcv
Copy link
Contributor

smcv commented Jan 13, 2022

What doesn't work?

I'm a Debian SDL maintainer, in the process of upgrading from SDL_ttf 2.0.15 to 2.0.18. When Debian's CI infrastructure runs the pysdl2 0.9.9 unit tests against SDL_ttf 2.0.18, they fail:

=================================== FAILURES ===================================
_________________________ TestSDLTTF.test_TTF_SizeText _________________________

self = <sdl2.test.sdlttf_test.TestSDLTTF object at 0x7feebf4df250>

    def test_TTF_SizeText(self):
        font = sdlttf.TTF_OpenFont(fontfile, 20)
        expected_w = 70
        expected_h = [
            25, # SDL2_ttf < 2.0.15
            24, # SDL2_ttf == 2.0.15 w/ FreeType 2.9.1
            21  # SDL2_ttf == 2.0.15 w/ FreeType 2.10.1
        ]
        w, h = c_int(0), c_int(0)
        sdlttf.TTF_SizeText(font, b"Hi there!", byref(w), byref(h))
>       assert w.value == expected_w
E       assert 69 == 70
E         +69
E         -70

sdl2/test/sdlttf_test.py:226: AssertionError
_________________________ TestSDLTTF.test_TTF_SizeUTF8 _________________________

self = <sdl2.test.sdlttf_test.TestSDLTTF object at 0x7feebf3fc460>

    def test_TTF_SizeUTF8(self):
        font = sdlttf.TTF_OpenFont(fontfile, 20)
        expected_w = 73
        expected_h = [
            25, # SDL2_ttf < 2.0.15
            24, # SDL2_ttf == 2.0.15 w/ FreeType 2.9.1
            21  # SDL2_ttf == 2.0.15 w/ FreeType 2.10.1
        ]
        w, h = c_int(0), c_int(0)
        sdlttf.TTF_SizeUTF8(font, u"Hï thère!".encode('utf-8'), byref(w), byref(h))
>       assert w.value == expected_w
E       assert 72 == 73
E         +72
E         -73

sdl2/test/sdlttf_test.py:240: AssertionError

Full log

I haven't tried this with 0.9.10 or git master, but the test in git master seems to have the same expected values as 0.9.9, so presumably the result will still be the same.

2.0.18 uses Harfbuzz for better support for non-Latin text and 2.0.15 did not, which might explain the different width; I don't think SDL_ttf aims to make any particular guarantee that text will render as exactly the same pixels in each version.

How To Reproduce

The test case that's run is basically this (slightly simplified here to avoid Debian-specific infrastructure):

#!/bin/sh

set -e

export SDL_VIDEODRIVER=dummy
export SDL_AUDIODRIVER=dummy
export SDL_RENDER_DRIVER=software

python3 -m pytest -v

Platform (if relevant):

  • OS: Debian unstable rolling release (will become Debian 12)
  • Python Version: 3.10
  • SDL2 Version: 2.0.18
  • Using pysdl2-dll: I don't know what this is, but our pysdl2 is built from source code
@a-hurst
Copy link
Member

a-hurst commented Jan 14, 2022

Hi @smcv, thanks for reporting this! I'm in the process of updating the PySDL2 bindings to support the new TTF release (there's a lot of new stuff!) so this will be fixed along with that, but I can also fix it in the main branch if that'll make things easier on Debian's end!

You're right that there isn't really a guarantee of identical rendering results between TTF versions (even the linked version of FreeType can change things unexpectedly), so I should probably rewrite those tests to be less specific and more future-proof.

As a side note, I had no idea PySDL2 had a Debian package, cool! I've been a big fan of the Debian-based Crunchbang distro for years. If there's anything else I can do as a maintainer to make your lives easier, please let me know!

smcv added a commit to smcv/py-sdl2 that referenced this issue Jan 14, 2022
SDL2_ttf 2.0.18 with Harfbuzz-based font rendering gives a width
slightly narrower than 2.0.15, so tolerate that.

Previously the height was not allowed to be 22 or 23, but if 21 and 25
are both acceptable values, then anything in between also seems fine;
for better future-proofing, accept a range.

Resolves: py-sdl#212
Signed-off-by: Simon McVittie <smcv@debian.org>
@smcv
Copy link
Contributor Author

smcv commented Jan 15, 2022

I'm in the process of updating the PySDL2 bindings to support the new TTF release (there's a lot of new stuff!) so this will be fixed along with that, but I can also fix it in the main branch if that'll make things easier on Debian's end

I need to get this test to pass in Debian before the new SDL_ttf release can be more widely available, because it's set up to be a QA gate to detect regressions, so it would be helpful to fix any regressions with the new release as a higher priority than fully supporting its new features. I hope #213 is suitable for that?

If there's anything else I can do as a maintainer to make your lives easier, please let me know!

I don't maintain the pysdl2 package myself (this is the first time I've looked at it) so I'll leave that to the people who are more closely associated with it! The developer-oriented overview of it is https://tracker.debian.org/pkg/pysdl2 if you're interested.

a-hurst pushed a commit that referenced this issue Jan 15, 2022
SDL2_ttf 2.0.18 with Harfbuzz-based font rendering gives a width
slightly narrower than 2.0.15, so tolerate that.

Previously the height was not allowed to be 22 or 23, but if 21 and 25
are both acceptable values, then anything in between also seems fine;
for better future-proofing, accept a range.

Resolves: #212
Signed-off-by: Simon McVittie <smcv@debian.org>
@a-hurst
Copy link
Member

a-hurst commented Jan 15, 2022

@smcv The other CI goof has been fixed and all runners are passing, so hopefully you're in good shape!

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 a pull request may close this issue.

2 participants