search

H-uru / Plasma

Cyan Worlds's Plasma game engine
http://h-uru.github.io/Plasma/
GNU General Public License v3.0
205 stars 81 forks source link

[Proposal] Refactor pfPython API classes #386

Open dpogue opened 10 years ago

dpogue commented 10 years ago

Using PyCXX (http://cxx.sourceforge.net/PyCXX-Python2.html) we can expose a Python API with much simpler, cleaner, C++-style code.

As an example, pyStream.h, pyStream.cpp, and pyStreamGlue.cpp would be reduced to just ptStream.h and ptStream.cpp and would look something like this: https://gist.github.com/dpogue/8502541

This proposal is to rewrite the existing Python API classes in that style and move them to an API subdirectory of pfPython.

As the classes are refactored, the exposed API should be audited and necessary additions/cleanups should be made. The option of adding attributes to make the API more Pythonic is also possible.

This ticket is to guage interest and hear any feedback/concern before actually trying to tackle the refactor.

Deledrius commented 10 years ago

This seems like a good idea and does appear cleaner. Before judgment, I think it might be useful to know how one of the more complicated interfaces would transfer, not just hsStream, just to be sure of the feasibility.

Also, will this help to address any existing issues or long-standing problems? This would be a big refactor, and not a crucial one AFAIK, so it would be excellent if there was more at stake than code simplification (which is still a useful goal).

Mystler commented 10 years ago

I think this sounds/looks pretty cool. The question is whether, like Deledrius said, this is really worth all of the work in the long run. The refactor should happen for the whole API, not just parts, and we'd need someone with the time to get it done.

dpogue commented 10 years ago

Okay, I've been looking at this a bit more, and there are a few things that stick out for me as annoyances.

  1. PyCXX doesn't have its own API for saying that a class has a base type. It's possible to do it using the normal C Python API and pulling a pointer from PyCXX (which could be easily wrapped in a #define), but it's worth mentioning.
  2. Getters/Setters for properties need to be implemented with getattr/setattr and strings. I don't know how much of a problem this is, since most Plasma stuff is using functions, but I'll play around a bit to see how bad it is.
  3. No PyCXX API for adding classes or constants to a module. You can do it by assigning named values to a dictionary object, but there's no 1 function call to register something. This is also easily fixed with a #define.
  4. Still need to do a bunch of plString conversion.

If we patch PyCXX, we can easily support 1 and 3, and probably 2, and contribute those patches upstream. On the other hand, if we don't mind having an in-tree customized PyCXX, we could change it to use plStrings directly and fix 4 as well.

branan commented 10 years ago

Forking and vendoring is the path to madness

dpogue commented 10 years ago

Forking and vendoring is the path to madness

I'm partly inclined to agree. I definitely think the first 3 issues should be fixed upstream. I'll make patches and try to figure out how to SourceForge.

Another thing I realized that might be an issue is the licence. It's BSD licenced, but has some additional clauses: https://sourceforge.net/p/cxx/code/HEAD/tree/trunk/CXX/COPYRIGHT Supposedly it's OSI compatible though.