[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [pygame] State of the math branch

To start with, it is great to see follow-through on this, it will be a great, long overdue, addition to the pygame api.
One thing I notice is that the main vector data structure contains a pointer to the actual coordinates. Other structures just contain the coordinates in an inline array up to the max dimension. IMO the main vector data structure should have the coordinates inline as well. This will cut in half the number of heap allocations to create a vector object. I suspect that it may be common for vector objects to be created as intermediate results in calculations and immediately discarded, and eliminating the extra memory management would be a helpful optimization, and simplify the code (less memory management is always a good thing). I think the only use-case for having the pointer would be for proxying an array of vector coordinates that you could operate on en masse (ala numpy).
A further optimization to consider would be an instance pool for vectors. If I'm right, and vectors are created and destroyed a lot in certain applications, having a pool of unused instances lying around could be a real win. This is how Python optimizes tuple object creation.
I didn't check thoroughly for this, but you need to be careful with code like this (in vector_elementwiseproxy_pow): 

    for (i = 0; i < dim; i++) {
        base = PyFloat_FromDouble(bases[i]);
        expo = PyFloat_FromDouble(expos[i]);
        result = PyNumber_Power(base, expo, Py_None);
        if (!result)
            return NULL;
        ret->coords[i] = PyFloat_AsDouble(result);
        Py_DECREF(result);
        Py_DECREF(expo);
        Py_DECREF(base);
    }
For one, PyFloat_FromDouble can return NULL, and I'm not sure what PyNumber_Power will do when passed a NULL arg. Assuming it doesn't crash and returns NULL, you may be leaking references to base or expo. It's probably safer to assume PyNumber_Power will crash however, which is very bad since it will mask the memory problem with a segfault. All of this conversion back and forth from Python floats seems unnecessary, however. Can't you just use the libc pow() function directly instead, saving all the PyFloat business?
I'm also curious what the use-case is for enabling and disabling swizzling? I know I've seen other libraries that have this switch (Euclid?), but why not just leave it on all of the time? Too error prone? 

-Casey

On Nov 14, 2009, at 4:45 AM, René Dudfield wrote:


Hi,

sweet :)  Nice work.

Feel free to merge it into the trunk.  It'll be easier for other
people to try it out(binaries, and other things hooked up with trunk),
and you can also use the build bot to check for errors on other
platforms(like compilation/testing).

Sorry, I haven't had a chance to review it much.  Hopefully will get a
chance to have a play, and a read of the code later this weekend some
time.

ps, here's the svn link for anyone else who would like to read over it
http://www.seul.org/viewcvs/viewcvs.cgi/branches/math_module/?root=PyGame&sortby=date


The (s)lerp idea with the iterator seems a good one.  I've been
thinking about doing something like that with line drawing
functions... that instead of drawing return the (x,y) coords of where
it would draw pixels.  I wonder if it still might be useful to let
people supply a t argument, just because people are used to it...
However, they could just supply one step.

Having them share implementations is probably a good thing to reduce
the complexity of the code.

I guess there is going to be a 4 element quaternion?  What about a
Vector4?  Which matrix sizes are you thinking of doing?  I guess 3x3,
3x4 and 4x4?  Or just 4x4?

I noticed one commit log said there was trouble with the buffer
interface?  Is this part still a trouble?



Functions like vector_elementwiseproxy_mul,
vector_elementwiseproxy_sub etc, could probably share a lot of code,
and then for the different part use a switch statement to do the
different function.  Something like this:

static PyObject *
vector_elementwiseproxy_mul(PyObject *o1, PyObject *o2) {
   return vector_elementwiseproxy_generic(o1, o2, VECT_OPS_MUL);
}

Where the vector_elementwiseproxy_generic function would have the
switch statement on the various flags passed in.




cheers,





On Thu, Nov 12, 2009 at 10:31 AM,  <don@xxxxxxxxxxxxxxxxx> wrote:


Hi List,
I just wanted to give you a short summery of the development in the math branch. The goal of the math branch is the inclusion of several math related types 
like vectors, matrices and quaternions.
Right now I consider the implementation of vectors in 2 and 3 dimensions as 
feature complete.
What this means is that I can't think of more functionality that I want to 
implement and all methods pass their unit tests.
I encourage everyone interested to take a look and make suggestions if they 
find functionality missing.

The current version is not written for maximum performance.
For example Vector2 and Vector3 share many functions so no dimension specific optimizations are implemented. So the current implementation should be 
considered a baseline for future optimizations.
To gauge future improvements I intend to write a rudimentary performance 
benchmark.

I don't consider the API to be set in stone. Especially concerning
mutability. Currently vectors are mutable.
If however it turns out that there is no significant performance hit in 
making them immutable I tend to do so.
Obviously this will only happen once I have some performance results.

After that Matrices will be up next.


thanks for your time and suggestions.

//Lorenz


PS: I feel that I should briefly comment on the slerp() method.
I did not follow the default implementation that seems to be prevalent on the Internet. There you repeatedly call the slerp method with a varying parameter t. I felt this is unpythonic. In my implementation you pass the number of steps you want into the method which then returns an iterator yielding the interpolating vectors. Same applies to the lerp() method. 
   Also the algorithm for slerp() is a bit different as to support
   interpolation to a vector of different length.