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

After PySDL2 update, getting function pointer for sdl2.sdlttf.TTF_SizeUTF8 breaks #218

Closed
ell1e opened this issue Jan 31, 2022 · 11 comments
Closed

Comments

@ell1e
Copy link

ell1e commented Jan 31, 2022

What doesn't work?
After PySDL2 update, getting a function pointer for sdl2.sdlttf.TTF_SizeUTF8 breaks. This is the Cython code that used to work before:

typedef int (*_sdl_TextSize_utf8_type)(
    void* font, char* text,
    int* resultw, int* resulth) nogil
cdef _sdl_TextSize_utf8_type _sdl_TextSize_utf8 = NULL


cdef tuple _get_font_size_fast_unthreaded(object sdl_font, char* text):
    global _sdl_TextSize_utf8
    if not _sdl_TextSize_utf8:
        import sdl2.sdlttf as sdlttf
        _sdl_TextSize_utf8 = <_sdl_TextSize_utf8_type>(
            cython.operator.dereference(<size_t*>(
            <size_t>ctypes.addressof(sdlttf.TTF_SizeUTF8))))  # line 70

With the update from PySDL 0.9.9 to 0.9.11, now this happens:

  File "src/myui/font/manager.pyx", line 99, in myui.font.manager.Font.render_size
  File "src/myui/font/sdlfont.pyx", line 189, in myui.font.sdlfont.get_thread_safe_render_size
  File "src/myui/font/sdlfont.pyx", line 70, in myui.font.sdlfont._get_font_size_fast_unthreaded
TypeError: invalid type

The documentation for PySDL2's SDL_TTF doesn't indicate any change here, so this seems to be a bug then...? Or otherwise I'm really not sure what is going on.

How To Reproduce
See Cython code above!

Platform (if relevant):

  • OS: Fedora 35
  • Python Version: 3.10.2
  • SDL2 Version: 2.0.20 (with SDL2_ttf 2.0.15)
  • Using pysdl2-dll: I don't know what that is, so probably not

Additional context
Add any other context about the problem here.

@ell1e
Copy link
Author

ell1e commented Jan 31, 2022

I did some back and forth testing, and it's broken in 0.9.10 and 0.9.11, and it works when going back to 0.9.9.

@a-hurst
Copy link
Member

a-hurst commented Feb 4, 2022

@ell1e Thanks for reporting this!

I think I know what's going on here, but I'm not sure what the easiest fix is: basically, between PySDL 0.9.9 and 0.9.10 I did some heavy refactoring of how the TTF, Image, Mixer, and GFX addon modules worked. Instead of exporting the ctypes bindings directly, the raw bindings are now stored in a dict and wrapped by thin pure-Python wrappers in order to provide autodoc, IDE docs, and kwargs support for the functions. I had no idea it was possible to use PySDL2 directly with Cython, so I guess that's why I didn't anticipate breaking anything!

Is it possible to access Python dicts from a Cython cdef? The raw ctypes bindings are all stored in a semi-private dict called "_funcs", with the keys being the function names:

https://github.com/marcusva/py-sdl2/blob/48a9f89f5c6192dea980a4deabe2ee5983cc0548/sdl2/sdlttf.py#L208-L210

So for instance,

from sdl2.sdlttf import _funcs as ttf_funcs
TTF_SizeUTF8 = ttf_funcs["TTF_SizeUTF8"]

should get you the same function pointer you'd previously get with from sdl2.sdlttf import TTF_SizeUTF8.

Since I've clearly made your development workflow more difficult accidentally (sorry!), would a get_func_pointer method that grabs the ctypes pointer for a given function name be helpful?

@ell1e
Copy link
Author

ell1e commented Feb 4, 2022

I'm not sure if I'd consider it helpful, although it would work. I think the ext module and indirections like this actually make the library less attractive for use in general. (Although the ext module at least is completely separate, so quite easy to ignore.) I always kind of thought PySDL2's point was to be a thin wrapper for those who mostly just stick to SDL2's own docs and/or know already how it works, not meant to provide some different API that is somehow pythonized. If the project is moving away from that, I wonder if I won't be the only person considering a switch in the medium term, it's just not what I'm interested in using. I picked PySDL2 because I was too lazy to do manual ctype bindings, not because I wanted something that isn't just plain SDL2 with no fuss.

While get_func_pointer works, anyone new to the library trying something similar probably won't easily find it or realize this is needed, so I'd rather suggest to just revert this wrapping. (Although if lots of people are relying on it by now, I imagine that could give those people trouble. Not sure what the best way forward is to help those, then.)

@a-hurst
Copy link
Member

a-hurst commented Feb 4, 2022

@ell1e Thanks for the feedback, that's definitely helpful. The purpose of PySDL2 (with the exception of ext) is still to provide thin ctypes bindings around SDL2, I just had no idea the bindings could be used like they are in your Cython example so I didn't think there was any trade-off to migrating to thin Python function wrappers (the wrappers are literally just there to allow for docstrings and argument names, they don't do extra stuff beyond call the raw binding). The official SDL docs for the addon modules are all extremely out of date and don't document any of the ctypes weirdness needed to use some of the functions, so the ability to integrate our own up-to-date docs with the bindings has been something the project's wanted for a long time.

Do you know of any other projects using PySDL2 this way? If this is somewhat common, I'd agree it might make sense to revert. If not, I'll make sure to properly document the way the bindings work and try to think of a better way of making the ctypes function pointers accessible for use cases like this.

@ell1e
Copy link
Author

ell1e commented Feb 4, 2022

I think the question should rather be if others generally use ctypes bindings this way, since that is what PySDL2 is (was?) advertised to be. I'd be very surprised if not!

Maybe you could add a function to ext that monkey patches all these wrappers in at runtime for those who desire it. This way, the default state of the basic plain modules as initially imported would remain clean.

Edit: additional note: if you need them in the original module just for documentation, then you probably need to use something else for documentation/figure out how to add this info in a different way in the doc string. E.g. doxygen afaik allows specifying named arguments manually without parsing them from the actual function header, if I recall right. There usually should be a way to do this that doesn't require actually having a matching argument header

@a-hurst
Copy link
Member

a-hurst commented Feb 5, 2022

Just so I understand the Cython code properly: instead of writing a Cython binding for TTF_SizeUTF8, you're using the ctypes function pointer from PySDL2, taking its address, and casting it through some process to a Cython-usable C function? I know of a few projects that use both Cython and PySDL2 for separate things (e.g. PyBoy), but this is the first I've seen them used together. Honestly, I'm amazed it's possible!

Anyway, I've talked it over with the other members of the maintenance team and I think I have a proposal. First of all, I'm sorry that my change broke your code without warning: that always sucks, and if I'd known this usage was in the realm of possibility I'd have held off the changes until the next major release (1.0). However, the fact is that outside of this narrow use-case (i.e. casting ctypes function pointers by address to Cython cdefs), the updated thin bindings behave exactly like before but bring some hefty quality-of-life benefits to most users (especially in terms of IDE integration, which always been a pain point with raw ctypes), so I'm not willing to revert back. Even if I did, PySDL2 has a lot of re-implemeted SDL macros as Python functions so this functionality would always be hit-or-miss and hard to document/maintain.

However, to offer a proper and stable API for your sort of use case (where you absolutely need the ctypes bindings to be actual ctypes pointers), I can make the raw binding pointers easily accessible by name via a "raw" object (basically a faux-submodule) that contains all the pointers as named attributes. This would involve no maintenance overhead on my part (I can auto-generate it with an extra 2 lines), but it would offer a simple and reliable API for this sort of non-standard usage. In practice, instead of import sdl2.sdlttf as sdlttf, you'd write from sdl2.sdlttf import raw as sdlttf and your code example should work exactly as before. Alternatively, you'd change sdlttf.TTF_SizeUTF8 to sdlttf.raw.TTF_SizeUTF8 and it'd similarly work fine.

Is that a reasonable option? I know it's not an ideal outcome for your project, but given that the vast majority of PySDL2 projects are using pure Python I need to look out for their interests first.

@ell1e
Copy link
Author

ell1e commented Feb 5, 2022

Edit: sorry, toned down the wording a little bit and I guess in the end it's still just my personal opinion. As written below I am kind of thinking that wouldn't be just me but who knows, I haven't done a survey

Your documentation literally says in the opening paragraph:

PySDL2 is a wrapper around the SDL2 library [...] it has no licensing restrictions, nor does it rely on C code, but uses ctypes instead.

And in the FAQ:

PySDL2 offers 1-to-1 bindings for most of the SDL2 API, keeping the same function names and arguments as the original SDL2, SDL2_mixer, SDL2_ttf, SDL2_image, and SDL2_gfx libraries.

I think the only reasonable expectation is that all functions that map to an SDL2 one therefore are ctypes objects. I can only reiterate that everything else just kinda flies in the face of what this is advertised as. I am also considering just doing my own manual ctypes mapping if that is no longer what PySDL2 is, and I highly recommend you change this paragraph if that is no longer what the project is aiming to be at its core, since I think that fundamentally alters what it is and it's misleading.

I don't think therefore this is reasonable, since it seems like a quite fundamental change. I also think it is very normal given this opening line of the documentation to expect functions to be ctypes objects, no matter what I use that knowledge for. That I happen to use it for Cython doesn't really matter, I think the expectation is pretty clear. I don't really see how you can justify breaking with that without surprising and disappointing new users in the future, and I think that is fundamentally a bad place to steer the project to.

@ell1e
Copy link
Author

ell1e commented Feb 5, 2022

I guess just to explain this further: when I picked up PySDL2, I actually checked carefully all it does is wrap via ctypes, therefore saves me the time to do a wrapper myself. So this is one of the core reasons I picked the project, to specifically not work with one that does a more complex, unnecessary wrapping. Unless that makes me the only one who picked it for that reason, I think this just isn't an expected thing to move away from.

But you know, maybe it is really just me. But these are my honest thoughts on it.

@a-hurst
Copy link
Member

a-hurst commented Feb 5, 2022

I don't think therefore this is reasonable, since it seems like a quite fundamental change. I also think it is very normal given this opening line of the documentation to expect functions to be ctypes objects, no matter what I use that knowledge for.

The bindings themselves are still 1-to-1 with the SDL2 API (and always will be), the point of the change isn't to make base PySDL2 any more "Pythonic" (that's ext's job, which is separate for a reason). Functions that require pass-by-reference arguments still require ctypes.byref, functions that return void pointers still require casting with ctypes, and so on. The only thing the thin Python wrappers add is the ability for the Python ecosystem to treat the functions as proper Python functions, which fixes a range of problems and limitations with autodoc, with linters, with kwargs, with autocompletion, etc.

Maybe I'm just not understanding the full scope of problems that the ctypes bindings not being actual ctypes function objects presents: are there other useful things end users can do with the raw function objects that they couldn't with a functionally-identical thin-wrapped binding? As you've shown there are clear use-cases where the raw ctypes objects are handy (hence why I proposed a stable API making them easily accessible to anyone who needs them), but apart from the accidental API-breaking the advantages of the primary bindings being thin-wrapped seem to far outweigh the costs (as I understand them) for most users.

@ell1e
Copy link
Author

ell1e commented Feb 8, 2022

Based on the lack of other comments maybe it's just me. I feel however that the approach is fundamentally wrong, if the IDEs need help then shouldn't it be written in some sort of annotation specifically fixing it for them rather than in some way affecting and breaking actual real world code? That just seems very backwards and like something to be fixed/provided on the IDE side instead. (E.g. by working in some convention to check doc comments for arguments, or such, into the IDE allowing you to do this properly.)

If you really must go with the hacky module approach, I feel like sdlttf.ctypes makes more sense than sdltypes.raw just based on what it is.

@a-hurst
Copy link
Member

a-hurst commented Jul 18, 2022

This is fixed in the latest PySDL2 release, which adds a formal and stable API for accessing the raw ctypes function objects for any non-macro or inline function in the library. The details are explained in the latest news.rst. Let me know if you encounter any issues with the new API!

@a-hurst a-hurst closed this as completed Jul 18, 2022
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

No branches or pull requests

2 participants