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

[wasm only] freetype 2 init and some cleanup #1967

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

pmp-p
Copy link
Member

@pmp-p pmp-p commented Mar 1, 2023

seems ok enough to run pygame_gui on browser.

@pmp-p pmp-p requested a review from a team as a code owner March 1, 2023 21:48
src_py/sysfont.py Outdated Show resolved Hide resolved
Copy link
Member

@MyreMylar MyreMylar left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@MyreMylar MyreMylar added this to the 2.2 milestone Mar 2, 2023
Copy link
Member

@oddbookworm oddbookworm left a comment

Choose a reason for hiding this comment

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

LGTM, but I'm not 100% comfortable enough with my understanding of the freetype module code to merge it, so I'm hoping that at least one other person could take a look

@pmp-p
Copy link
Member Author

pmp-p commented Mar 6, 2023

@oddbookworm it's quite safe, as the real code change is fenced for wasm, the rest is really only cosmetic removing the confusion beetween font "name" and font "file" and "module" vs "module definition"

Though usefullness of the whole module itself is still discutable. Especially on mobile without some tools/documentation to generate the font cache.

tbh if no module else than pygame_gui still use it, it could go away
using OS fonts is just working around problem of free font distribution in an akward and non portable way.

@Starbuck5
Copy link
Member

Starbuck5 commented Mar 7, 2023

Why do you need to initialize the freetype library instance in module initialization rather than in pygame.freetype.init? In the same vein, why can't it use the normal code for freetype.get_version?

@pmp-p
Copy link
Member Author

pmp-p commented Mar 7, 2023

@Starbuck5 the static init is a bit different and now keeps a global reference so that other subfunctions can reach the pointer. If module init was done like before the ref would be unreachable as soon as leaving the scope of module init

@Starbuck5
Copy link
Member

I went ahead and pushed a fix for the build failures, since it was caused by a PR of mine getting merged.

@Starbuck5 the static init is a bit different and now keeps a global reference so that other subfunctions can reach the pointer. If module init was done like before the ref would be unreachable as soon as leaving the scope of module init

I don't understand. It looks like you use the freetype module state to store two others things perfectly fine (cache_size and resolution), why can't you store a freetype library pointer there and have it get initialized in _ft_autoinit?

My motivation for caring is that the less special case code for emscripten is needed throughout pygame, the easier things will be to maintain in the future.

@pmp-p
Copy link
Member Author

pmp-p commented Mar 11, 2023

iirc _ft_init() was not able to initialize properly on wasm, and i did not investigate much but i'll have another look. Though as said earlier I'm not sure of usefullness of that module as I think using os fonts is doomed.

@pmp-p
Copy link
Member Author

pmp-p commented Mar 13, 2023

i probably used a bad test the init looks ok, tested with pygame_gui

src_c/_freetype.c Outdated Show resolved Hide resolved
src_c/_freetype.c Outdated Show resolved Hide resolved
src_c/_freetype.c Outdated Show resolved Hide resolved
src_py/sysfont.py Outdated Show resolved Hide resolved
@Starbuck5 Starbuck5 modified the milestones: 2.2, 2.3 Mar 26, 2023
@Starbuck5
Copy link
Member

So @pmp-p I still have reservations about the two places EMSCRIPTEN is used in the freetype C code, I have an unsubstantiated hunch that the code would work fine with the non-emscripten code paths.

If you just want to get this PR merged without testing that, I would be fine if there were comments in the code explaining that. Like "it's not that getting the linked version doesn't work on emscripten, it's just that it hasn't been tested." I just don't want some future maintainer to see those EMSCRIPTEN blocks and assume that the non-emscripten code definitely won't work on web for some reason.

@pmp-p
Copy link
Member Author

pmp-p commented Mar 29, 2023

@Starbuck5 you are right about the blocks, but i kinda hoped to target wasi just by pretending being in emscripten as i have a wasi SDL2 port with sixel/framebuffer output as a side project.

also i need more time for testing i'm quite busy with real life atm.

@Starbuck5 Starbuck5 modified the milestones: 2.3, 2.3.1 Jun 10, 2023
@ankith26 ankith26 removed this from the 2.3.1 milestone Jul 28, 2023
@pmp-p
Copy link
Member Author

pmp-p commented Sep 19, 2023

it seems that PR alone is not sufficient to init SDL_ttf correctly on wasm ( when dynamic / side module ), static has no problem. But the goal is to make a wheel

it seems TTF_Init() is not called at all. I had to add it directly in PyInit_pygame_static() of src_c/static.c to get it effective ...

it's a blocker for pyodide/pyodide#289

@pmp-p pmp-p changed the title wasm freetype 2 init and some cleanup [wasm only] freetype 2 init and some cleanup Sep 29, 2024
@pmp-p pmp-p self-assigned this Sep 30, 2024
@pmp-p pmp-p linked an issue Sep 30, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sysfont (3432) on non-os platforms
6 participants