[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [pygame] SDL_gfx integration
Thanks a lot!
I just looked through your patch and I have some remarks:
1) In line and aaline you got the argument names mixed up:
if(!PyArg_ParseTuple(arg, "O!(ii)(ii)O", &PySurface_Type, &surfobj, &x1, &x2, &y1, &y2, &colorobj))
should be
if(!PyArg_ParseTuple(arg, "O!(ii)(ii)O", &PySurface_Type, &surfobj, &x1, &y1, &x2, &y2, &colorobj))
and accordingly
lineRGBA(surf, x1, x2, y1, y2, rgba[0], rgba[1], rgba[2], rgba[3]);
should change to
lineRGBA(surf, x1, y1, x2, y2, rgba[0], rgba[1], rgba[2], rgba[3]);
2) you grouped the arguments to horizontal and vertical line like this:
(surface, (x1, x2), y, color) and (surface, x, (y1, y2), color) respectively
I find this grouping not very intuitive. normally we group a pair of x and y
coordinates which makes sense because they represent one point but I can't see
such an interpretation here. one could argue to group them (surface, (x1, y),
x2, color) but that looks strange as well so I would suggest to not group them
at all
3) this is the biggest point and concerns the polygon related functions so I
post it here as reference:
static PyObject*
polygon (PyObject* self, PyObject* arg)
{
PyObject *surfobj, *colorobj, *points;
SDL_Surface* surf;
Uint8 rgba[4];
if(!PyArg_ParseTuple(arg, "O!OO", &PySurface_Type, &surfobj, &points, &colorobj))
return NULL;
surf = PySurface_AsSurface(surfobj);
if(!RGBAFromColorObj(colorobj, rgba))
return RAISE(PyExc_TypeError, "invalid color argument");
if(!PySequence_Check(points))
return RAISE(PyExc_TypeError, "points argument must be a sequence of number pairs");
int length = PySequence_Length(points);
if(length < 3)
return RAISE(PyExc_ValueError, "points argument must contain more than 2 points");
PyObject *item = PySequence_GetItem(points, 0);
int loop, x, y;
int result = TwoIntsFromObj(item, &x, &y);
Py_DECREF(item);
if(!result) return RAISE(PyExc_TypeError, "points must be number pairs");
Sint16 *xlist = PyMem_New(Sint16, length);
Sint16 *ylist = PyMem_New(Sint16, length);
int numpoints = 0;
for(loop = 0; loop < length; ++loop)
{
item = PySequence_GetItem(points, loop);
result = TwoIntsFromObj(item, &x, &y);
Py_DECREF(item);
if(!result) continue; /*note, we silently skip over bad points :[ */
xlist[numpoints] = x;
ylist[numpoints] = y;
numpoints++;
}
polygonRGBA(surf, xlist, ylist, numpoints, rgba[0], rgba[1], rgba[2], rgba[3]);
PyMem_Del(xlist); PyMem_Del(ylist);
Py_RETURN_NONE;
}
so here come my two cents:
why do you check the first point in the sequence and require it to be a pair of numbers but silently skip over the others?
why do you silently skip over bad points in the first place? is it a good thing to be so permissive?
I believe pygame is trying to conform to the c89 standard like CPython. If that is the case,
isn't it required that local variables be declared at the beginning of a code block?
I realized that this part of the code shows a strong resemblence to some code in draw.c (around line 550)
so my questions go out to the author of that code as well.
This would be my proposed version of the same function:
static PyObject*
polygon (PyObject* self, PyObject* arg)
{
PyObject *surfobj, *colorobj, *points;
SDL_Surface* surf;
Uint8 rgba[4];
int length, loop;
Sint16 *xlist, *ylist;
if(!PyArg_ParseTuple(arg, "O!OO", &PySurface_Type, &surfobj, &points, &colorobj))
return NULL;
surf = PySurface_AsSurface(surfobj);
if(!RGBAFromColorObj(colorobj, rgba))
return RAISE(PyExc_TypeError, "invalid color argument");
if(!PySequence_Check(points))
return RAISE(PyExc_TypeError, "points argument must be a sequence of number pairs");
length = PySequence_Length(points);
if(length < 3)
return RAISE(PyExc_ValueError, "points argument must contain more than 2 points");
xlist = PyMem_New(Sint16, length);
ylist = PyMem_New(Sint16, length);
for(loop = 0; loop < length; ++loop)
{
int x, y;
PyObject *item = PySequence_GetItem(points, loop);
int result = TwoIntsFromObj(item, &x, &y);
Py_DECREF(item);
if(!result)
{
PyMem_Del(xlist);
PyMem_Del(ylist);
return RAISE(PyExc_TypeError, "points must be number pairs");
}
xlist[loop] = x;
ylist[loop] = y;
}
polygonRGBA(surf, xlist, ylist, length, rgba[0], rgba[1], rgba[2], rgba[3]);
PyMem_Del(xlist);
PyMem_Del(ylist);
Py_RETURN_NONE;
}
so thats all for now. thanks again for the patch and I'm looking forward to a response.
sincerely yours
//Lorenz
Forrest Voight wrote:
> Hi!
>
> I'm posting the patch at http://im.forre.st/pygame-sdl_gfx.diff , so
> that won't happen again.
>
> It's pretty much a full binding of the shape drawing functions, but
> there's some other stuff in SDL_gfx.
>
> I think there is some kind of licensing problem, I'm not sure.
>
> There was talk about putting it into the pygame tree, or making it a
> requirement. Making it optional isn't really an option.
>
> Also, it would probably be combined into the draw module instead of
> being in its own module.
>
> On Sat, Oct 25, 2008 at 6:00 AM, Lorenz Quack <don@xxxxxxxxxxxxxxxxx> wrote:
>> Hi pygamers,
>>
>> I was reading the mailinglist archive the other day and found two threads ([1]
>> and [2]) about integrating SDL_gfx into pygame.
>>
>> So my questions are:
>> 1) Is someone still working on this?
>> 2) How far are you with your efforts?
>> 3) Is there a place I could take a look at the progress like a svn branch?
>> 4) Is there anyway I can help out with this?
>>
>> sincerely yours
>> //Lorenz
>>
>> PS: unfortunately the patch isn't preserved in the gmane archive so I can't
>> even look at that :(
>>
>>
>> [1] http://thread.gmane.org/gmane.comp.python.pygame/14790/
>> [2] http://thread.gmane.org/gmane.comp.python.pygame/15846/
>>