search

pygame-community / pygame-ce

🐍🎮 pygame - Community Edition is a FOSS Python library for multimedia applications (like games). Built on top of the excellent SDL library.
https://pyga.me
923 stars 152 forks source link

Refactoring Pygame C API (333) #347

Closed GalacticEmperor1 closed 1 year ago

GalacticEmperor1 commented 1 year ago

Issue №333 opened by llindstrom at 2017-04-04 00:50:44

Pygame has a C API, though it is undocumented, incomplete, and inconsistent. Since the API is only used internally—no external package depends on it—then it should be safe to alter. Cleaning up and filling out the C API, as well as properly documenting it, will help in maintenance of and extension to the package. Here are suggests:

Comments

*bitcraft commented at 2017-04-04 06:06:58*

+1 Remove GAME_Rect

*illume commented at 2017-04-04 07:21:17*

Consistent use of pymalloc would be nice too. Also, for objects like events and Rects which we allocate a lot, using a pymalloc arena might be nice. https://docs.python.org/3.6/c-api/memory.html# the-pymalloc-allocator It's much faster for such small objects than malloc. But yeah, defining a consistent approach and documenting it would be good.

I'd suggest we start documenting the C API as we go through the modules one by one with the SDL2 conversion.

I'm not sure if we should add something like this: http://www.pygame.org/wiki/Hacking to the docs?

*illume commented at 2017-04-13 00:57:01*

PYG, pyg prefix?

*bitcraft commented at 2017-04-13 01:02:39*

PG would be nice. On Wed, Apr 12, 2017 at 7:57 PM René Dudfield notifications@github.com wrote:

PYG, pyg prefix?

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub <https://github.com/pygame/pygame/issues/333# issuecomment-293747551>, or mute the thread https://github.com/notifications/unsubscribe-auth/AAh-p-7Kf_mAtB3kfvOTTQMbFZ7LpBhjks5rvXLdgaJpZM4MyQ9Y .

*illume commented at 2017-04-13 10:33:51*

Yeah, I think PG is good. For some reason I always associate that with postgresql, so that's perhaps why my subconscious avoided it.

*illume commented at 2017-04-13 12:28:28*

Here's the numpy C-API docs, and their include files. https://docs.scipy.org/doc/numpy/reference/c-api.types-and-structures.html https://github.com/numpy/numpy/tree/master/numpy/core/include/numpy

The latest Python C API documentation. https://docs.python.org/3/c-api/intro.html

*llindstrom commented at 2017-04-13 16:17:24*

Going back to /PEP 7 -- Style Guide for C Code/, and the /Extending Python with C or C++/ chapter of the Python 3.6 documentation I find no suggestion to avoid the Py prefix in third party code. So its use is not discouraged, at least not formally. While using a different prefix guarantees no namespace conflict, in practice I see no problem. So I go with either Py or PG, just not the Pg I introduced and now find ugly.

*llindstrom commented at 2017-04-13 16:32:46*

In SDL 2 SDL_Rect is equivalent to GAME_Rect, so GAME_Rect will go away. They differ for SDL 1 so it will remain there.

*illume commented at 2017-04-13 16:51:59*

The Py avoidance thing is mentioned here: https://docs.python.org/3/c-api/intro.html# include-files

Numpy uses npy or NPY depending on usage situation.

music_get_busy becomes pg_music_get_busy I think that sort of looks ok?

*llindstrom commented at 2017-04-13 20:23:49*

But PgBufproxy_New? It tries too hard to be PyBufproxy_New. PGBufproxy_New is more distinct. Still, I am reminded of the PG movie rating: Parental Guidance is required to use this code. Though more verbose, Game works better for me: GameBufproxy_New, GameSurfaceObject, Game_TwoFloatsFromObj. As for music_get_busy that is internal to music.c. So does it need a the pg, or some other, prefix?

*llindstrom commented at 2017-04-13 20:32:41*

I find Gm is also ascetically appealing: GmBufproxy_New, Gm_TwoFloatsFromObj. It lacks the descender of Pg.

*illume commented at 2017-04-13 20:37:22*

Good find.

*illume commented at 2017-04-13 20:45:11*

I'm trying to check out the latest in symbols. I thought all of them were exported. But it seems there's still a mixture of platforms. Some require you set visibility, and others export everything.

We haven't had any clashes all these years... so I don't think it's such a big deal for music_get_busy / gm_music_get_busy? On the other hand, maybe it would be cleaner to do so.

(gm reminds me of dungeon master. Which makes it an extra good choice IMHO.)

*llindstrom commented at 2017-04-13 20:49:18*

Sure, we can prefix all our C globals, whether they get exported or not.

*bitcraft commented at 2017-04-13 22:54:31*

"pg" is used often to shorten the pygame import in community projects. PYG sounds like "Pig" and idk, reminds me of Pyglet, so I don't feel it is appropriate. GM is kinda recognized as Game Maker, a commercial project. On Thu, Apr 13, 2017 at 3:49 PM Lenard Lindstrom notifications@github.com wrote:

Sure, we can prefix all our C globals, whether they get exported or not.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub <https://github.com/pygame/pygame/issues/333# issuecomment-294017734>, or mute the thread https://github.com/notifications/unsubscribe-auth/AAh-p2lvtzcua3OTAUvECrRRyIxwAd2rks5rvopPgaJpZM4MyQ9Y .

*illume commented at 2017-04-14 12:33:14*

A couple of suggestions for handling the # ifndef SDL2s...

1) I'm not sure I like how it is usually # ifndef SDL2. It's good because the SDL1 stuff goes first. However, I usually remove if not style logic, as it is less logic. Additionally, I don't like how SDL2 is written there, but that bit isn't SDL2. IMHO it would be more readable with # ifdef SDL2.

2) For the # ifndef SDL2 stuff, where there are lots of them in one function, I think it's better if we have two separate sections. So that reading the logic of the function is easier. A second benefit is that it is easier to see if the SDL1 function is the same as it was before. This is important because we don't want to introduce errors into the SDL1 code.

Interspersed ifdefs all together in one function...

static PyObject*
flip (PyObject* self)
{
# ifndef SDL2
    SDL_Surface* screen;
# else /* SDL2 */
    SDL_Window* win = Py_GetDefaultWindow ();
# endif /* SDL2 */
    int status = 0;

    VIDEO_INIT_CHECK ();

# ifndef SDL2
    screen = SDL_GetVideoSurface ();
    if (!screen)
# else /* SDL2 */
    if (!win)
# endif /* SDL2 */
        return RAISE (PyExc_SDLError, "Display mode not set");

    Py_BEGIN_ALLOW_THREADS;
# ifndef SDL2
    if (screen->flags & SDL_OPENGL)
        SDL_GL_SwapBuffers ();
# else /* SDL2 */
    if (SDL_GetWindowFlags (win) & SDL_WINDOW_OPENGL)
        SDL_GL_SwapWindow (win);
# endif /* SDL2 */
    else
# ifndef SDL2
        status = SDL_Flip (screen) == -1;
# else /* SDL2 */
        status = SDL_UpdateWindowSurface (win) == -1;
# endif /* SDL2 */
    Py_END_ALLOW_THREADS;

    if (status == -1)
        return RAISE (PyExc_SDLError, SDL_GetError ());
    Py_RETURN_NONE;
}

VS separate function blocks for each one...

static PyObject*
gm_flip (PyObject* self)
{
# ifdef SDL2
    SDL_Window* win = Py_GetDefaultWindow ();
    int status = 0;

    VIDEO_INIT_CHECK ();

    if (!win)
        return RAISE (PyExc_SDLError, "Display mode not set");

    Py_BEGIN_ALLOW_THREADS;
    if (SDL_GetWindowFlags (win) & SDL_WINDOW_OPENGL)
        SDL_GL_SwapWindow (win);
    else
        status = SDL_UpdateWindowSurface (win) == -1;
    Py_END_ALLOW_THREADS;

    if (status == -1)
        return RAISE (PyExc_SDLError, SDL_GetError ());
    Py_RETURN_NONE;
# else
    SDL_Surface* screen;
    int status = 0;

    VIDEO_INIT_CHECK ();

    screen = SDL_GetVideoSurface ();
    if (!screen)
        return RAISE (PyExc_SDLError, "Display mode not set");

    Py_BEGIN_ALLOW_THREADS;
    if (screen->flags & SDL_OPENGL)
        SDL_GL_SwapBuffers ();
    else
        status = SDL_Flip (screen) == -1;
    Py_END_ALLOW_THREADS;

    if (status == -1)
        return RAISE (PyExc_SDLError, SDL_GetError ());
    Py_RETURN_NONE;
# endif
}

*illume commented at 2017-04-14 12:36:26*

ps. I'd probably prefer gm(shorter). But am happy with game, or anything else really(pg, pyg, p2, pg2, gargleblartfast). @llindstrom can you choose?

*bitcraft commented at 2017-04-14 12:41:41*

The # 2 option with longer blocks of code in the ifndef looks much cleaner IMO. Probably no hard rule, but when applicable, that approach looks less ugly. On Fri, Apr 14, 2017 at 7:36 AM René Dudfield notifications@github.com wrote:

ps. I'd probably prefer gm(shorter). But am happy with game, or anything else really(pg, pyg, p2, gargleblartfast). @llindstrom https://github.com/llindstrom can you choose?

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub <https://github.com/pygame/pygame/issues/333# issuecomment-294146609>, or mute the thread https://github.com/notifications/unsubscribe-auth/AAh-pxIbFIujZJSSEabFO1jAD8uExdDFks5rv2hKgaJpZM4MyQ9Y .

*illume commented at 2017-04-14 13:51:44*

numpy uses the NPY_VISIBILITY_HIDDEN macro, which is compiler dependent. (eg, __attribute__((visibility("hidden"))) ). They also have a separate private module... for private things. That's probably too much work to implement.

I guess we just put what we mean to be a public API in the docs for now.

*illume commented at 2017-04-14 14:23:58*

Visual Studio now supports a bunch of C99. Most importantly C99 variable declarations. Which means we don't need to have everything at the top anymore. Except that we need to support python 2.7. Only python3.5+ on windows use a new enough compiler.

I was just considering using this to get rid of a warning in display.c, but I don't think we can yet.

Python windows compilers: https://wiki.python.org/moin/WindowsCompilers# Which_Microsoft_Visual_C.2B-.2B-_compiler_to_use_with_a_specific_Pythonversion.3F

https://blogs.msdn.microsoft.com/vcblog/2013/07/19/c99-library-support-in-visual-studio-2013/ https://blogs.msdn.microsoft.com/vcblog/2013/06/28/c1114-stl-features-fixes-and-breaking-changes-in-vs-2013/ https://msdn.microsoft.com/en-us/library/hh409293.aspx https://blogs.msdn.microsoft.com/vcblog/2017/03/07/visual-studio-2017-for-c-developers-you-will-love-it/

• C99 _Bool • C99 compound literals • C99 designated initializers • C99 variable declarations

*illume commented at 2017-04-14 15:29:29*

I started a pull request for display.c merging into SDL2_patches # 335

*llindstrom commented at 2017-04-14 18:20:11*

The conditional SDL2 code was generated using diff. It has no understanding of the underlying C code. It was a quick and dirty way of getting things started. I plan to go through the code and clean up the # ifdef's and # ifndef's. I have a local changes to SDL_patches that adds the IS_SDLv1 and IS_SDLv2 boolean macros which work like PY2 and PY3. So all those # ifndef SDL2 and # ifdef SDL2 lines become # if IS_SDLv1 and # if IS_SDLv2 respectively. The macro names are open to negotiation.

*llindstrom commented at 2017-04-14 18:49:59*

I thing pg, lower case, is the better prefix: pg_TwoIntsFromObj, pgSurface_Type, pg_toggle_fullscreen. It is visually distinct from Py, stands out in Camel Case, and, as @bitcraft noted, pg is a recognized abbreviation for pygame in Python code.

*illume commented at 2017-04-16 09:14:08*

Great, pg does look pretty nice.

I'm finding it quite slow going manually cleaning up the SDL2 define blocks. I found it easier to look at the master branch for the SDL1 part, and go from there. I'll set up another branch with the SDL2 patches applied without ifdefs. Of course, display.c is one of the ones with the most changes(surface.c, alphablit.c, event.c, surface_fill.c).

You can see how many SDL2 defines are in which files with this: find . -name \*.c -exec grep -H -c SDL2 {} \;

With nm -g bla.so, (on ubuntu 16.04, python35 at least) it seems the symbols are not exported globally by default. To see locally exported symbols on linux use readelf -Ws bla.so. On mac use nm bla.so (mac doesn't have the concept of local and global exports).

*llindstrom commented at 2017-04-16 19:15:18*

There are two new C macros, IS_SDLv1 and IS_SDLv2, which are always defined booleans like PY2 and PY3. So each # ifdef SDL2 and # ifndef SDL2 can be replaced with a # if IS_SDLv2 or a # if IS_SDLv1 respectively.

*llindstrom commented at 2017-04-16 19:26:32*

For now I will continue working on src/base.c and its exported api in src/_pygame.h.

*llindstrom commented at 2017-04-18 18:45:56*

I have added some temporary tools to Pygame SDL2_patches branch to help in the API refactoring. The shell script src/update-api script replaces old API names in a file passes as an argument. It currently does updates for the API exported from src/base.c. As more source files are updated, add the changes to src/update-api.sed.

A temporary header file src/api_transition.h, which is included at the top of src/_pygame.h, defines macros for the old API names, so changes can be made a file at a time. Like src/update-api.sed, add new defines to src/api_transition.h for each refactored source file.

*illume commented at 2017-04-24 05:32:48*

Nice. api_transition was a good idea.

*illume commented at 2019-01-05 10:50:01*

Thinking of closing this because there isn't any list of what remains to be done. There's a number of files that still need to be 'modernized' but they can be done as we work on them.

*illume commented at 2019-01-05 10:51:16*

Changed this to 2.1 milestone because I don't think it should block the release of pygame 2.

MyreMylar commented 1 year ago

Closing this as there doesn't seem to be any work left here to do after checking through it.

This issue was mostly concerned with issues surrounding the transition between SDL1 & SDL2.