Port Renderer to C code

[MightyJosip]

This PR converts pygame._sdl2.video.Renderer to C code. It has the implementation of all the Renderer methods and attributes. It doesn't have docs/type hints/tests or stuff like that.

[Starbuck5]

Pulled the relevant bit of logs for the WASM build failure:

src_c/render.c:412:19: error: call to undeclared function 'pgSurface_New'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
        surface = pgSurface_New(surf);
                  ^
src_c/render.c:412:19: note: did you mean 'pgSurface_New2'?
src_c/surface.c:343:1: note: 'pgSurface_New2' declared here
pgSurface_New2(SDL_Surface *s, int owner)
^
In file included from src_c/static.c:372:
src_c/render.c:412:17: error: incompatible integer to pointer conversion assigning to 'pgSurfaceObject *' from 'int' [-Wint-conversion]
        surface = pgSurface_New(surf);
                ^ ~~~~~~~~~~~~~~~~~~~

[MightyJosip]

Fixed

[Starbuck5]

I feel like this code could be simpler.
It checks a property, grabs the function, checks if its a callable, then calls it by looking it up again. -- Wouldn't PyUnicode_FromString("draw") leak?

If you're going to check that it's callable first you should use https://docs.python.org/3/c-api/call.html#c.PyObject_CallFunctionObjArgs because you've already looked it up you don't need to look it up again.

But I'm sorta thinking instead of fetching it, checking it, and then calling it you could just try to call it? Let Python do those checks for you.

[MightyJosip]

Ok I changed it, and when testing it looks kinda ok. Can you check if that is what you meant.

[Starbuck5]

If you stored the None object instead of NULL you could have a unified codepath here. Not sure if that makes it harder other places though.

[MightyJosip]

Yea I see what you meant, and here indeed it would be better. Though the target in pgRendererObject is defined as pgTextureObject, and I think it is better than PyObject that could then accept Py_None. I would go with the current version, though if you want I can change it to PyObject

[Starbuck5]

I'm suspicious of this incref. We're not taking ownership of the surface, we just are using it randomly in this function.

[MightyJosip]

It is needed because in line 410 we return that surface. In the else branch (without passing surface as the argument to that function) it works ok, because I guess pgSurface_New() increases the ref, so I put it only in that if branch. Without it, the following code

surface = pygame.Surface((100, 100))
window = pygame.Window(size=(100, 100))
renderer = _render.Renderer(window)
renderer.to_surface(surface)
print(surface)

segfaults on print(surface) line (or basically any call of the surface)

[damusss]

I hope this doesn't sound nitpicky. In this draw and fill methods depending on the amount of points there is a great repetition of the pg_TwoFloatsFromObj code with an increasing vertex index and later the repetition of vertices[index].color = color. I think both of them could be put into C loops for all of the methods. If you don't want to make a temporary array with p1, p2, p3... you can create a temporary macro before this methods and undefine it after, since that code pattern is used a lot around here. You can also ignore all of this, it's only about my biased view on code quality.

[MightyJosip]

Ofc, it is not nitpicky, I am always happy for good review. Yea I agree with you, I changed it a bit, can you check if it is something you also thought or you would like some extra changes

[damusss]

🔽