Page MenuHome

BGE: generic python callback list + replace KX_PythonSeq.
ClosedPublic

Authored by Porteries Tristan (panzergame) on Jun 18 2015, 9:51 AM.

Details

Summary

I made this patch to declared a python list without converting all elements in python object (too slow) or use a CListValue which required CValue items (too expensive in memory). In the case of a big list of points like a collision contacts points list, to use a CListValue we must implement a new class based on CValue for 3D vector to create a python proxy even if mathutils do it perfectly, we must also convert all points (frequently ~100 points) when fill the CListValue even if the list is not used (in the case of the collision callback). The easy way is to use callback (it doesn't worth to do an inheritance) which convert the item in PyObject only during an acces.
5 callbacks are used :

  • Check if the list is valid = allow acces (like PyObjectPlus.invalid)
  • Get the list size
  • Get an item in the list by index.
  • Get an item name in the list by index (used for operator list["name"])
  • Set an item in the list at the index position.

All of these callback take as first argument the client instance.
Why do we use a void * for the client instance ? : In KX_PythonInitTypes.cpp we have to initialize each python inherited class, if we use a template (the only other way) we must add this class each time we use a new type with in KX_PythonInitTypes.cpp

To check if the list can be accessed from python by the user, we check if the python proxy, which is the m_base member, is still a valid proxy like in PyObjectPlus. But we can use a callback for more control of user access (e.g a list of collision point invalidate a frame later, in this case no real python owner).

This python list is easily defined with :

CPythonCallBackList(
void *client, // The client instance
PyObject *base, // The python instance which owned this list, used to know if the list is valid (like in KX_PythonSeq)
bool (*checkValid)(void *), // A callback to check if this list is till valid (optional)
int (*getSize)(void *), // A callback to get size
PyObject *(*getItem)(void *, int), // A callback to get an item
const char *(*getItemName)(void *, int), // A callback to get an item name (optional) use for acces by string key
bool (*setItem)(void *, int, PyObject *) // A callback to set an item (optional)
)

To show its usecase i replaced the odd KX_PythonSeq, it modify KX_Gameobject.sensors/controllers/actuators, SCA_IController.sensors/actuators and BL_ArmatureObject.constraints/channels.

Example :

, See message in console, press R to erase the object and see invalid proxy error message.

Diff Detail

Repository
rB Blender
Branch
ge_python_callback_list

Event Timeline

Sergey Sharybin (sergey) requested changes to this revision.Jun 19 2015, 9:56 AM

With patches like that it's crucial to provide at least minimal information about how users will benefit from it and how to use it. In this particular patch it's a bit more tricky because the class seems to be declared by nowhere used?

source/gameengine/Expressions/ListPython.h
47 ↗(On Diff #4438)

Not sure why do you make every method virtual in this class. From the current class structure it's really only GetName() and SetName() which could be declared virtual here.

All the rest you should keep non-virtual unless there's really good reason to make them virtual.

This revision now requires changes to proceed.Jun 19 2015, 9:56 AM

Not sure this is needed?

Why not just add iterator type for PyTypeObject CListValue::Type, see bpy_bmelemseq_iter ?

Porteries Tristan (panzergame) edited edge metadata.

@Campbell Barton (campbellbarton): CListValue is a real mystery, it doesn't have any iterator functions but it can be iterable =/.

Porteries Tristan (panzergame) edited edge metadata.

Refractor python generator. Now we avoid duplication by using as mapping python operator.

Porteries Tristan (panzergame) edited edge metadata.
  • Implement a python calbback list and replace KX_PythonSeq.
Porteries Tristan (panzergame) retitled this revision from BGE: generic python generator. to BGE: generic python callback list + replace KX_PythonSeq..Oct 11 2015, 7:22 PM
Porteries Tristan (panzergame) updated this object.
Porteries Tristan (panzergame) updated this object.
  • Decref base and allow base to be NULL, use getName in BL_ArmatureChannel instead of indrection GetPoseChannel()->name

Note, I found the name CPythonCallBackList confusing. (though to be fair, this is rather complicated concept to try fit into short name).

The callback part of this is IMHO an implementation detail. The same way we don't call a mathutils.Vector a mathutils.CPythonCallBackVector... Its just not so useful IMHO.

How about CListValueWrap? - Theres not necessarily any need to have Python in the name.
CValue and CList have python interfaces too, but we don't include CPython in their names.

source/gameengine/Converter/BL_ArmatureObject.cpp
649–659

Local callbacks - prefer to keep them lower-case. see mathutils_rna_array_cb

source/gameengine/Expressions/EXP_PythonCallBackList.h
97–101

python_callback_list_*** prefix all over seems a bit verbose?

Could be more brief py_buffer_len_cb, py_buffer_get_item_cb... etc.

What do you think about CListWrapper ? (ClistValueWrapper will say that we use CValue type in)

sure, CListWrapper is fine too.

  • Rename CPythonCallbackList to CListWrapper, rename static callbacks.
Campbell Barton (campbellbarton) edited edge metadata.

LGTM.

source/gameengine/Expressions/EXP_ListWrapper.h
23 ↗(On Diff #5198)

not matching filename.

29–30 ↗(On Diff #5198)

Convention for include guards is based on filename. __EXP_LISTWRAPPER_H__

92–101 ↗(On Diff #5198)

could use py_ prefix here since these are py_ specific.

  • Replace list_ prefix by py_ prefix, fix doxygen file name and include macro name.
Mitchell Stokes (moguri) requested changes to this revision.Oct 15 2015, 6:41 PM
Mitchell Stokes (moguri) edited edge metadata.

It looks like this new class does not define a get() method, but KX_PythonSeq did. This will break scripts that relied on KX_PythonSeq.get().

source/gameengine/Converter/BL_ArmatureObject.cpp
623

I'm not sure I like the use of the name "client." Our Bullet/Physics code tends to use "client," but else where we tend to use "self" (e.g., in Python code and Mathutils callbacks).

669

There seems to be some odd indentation throughout these changes. Maybe Differential is not displaying them right? If this is the actual indentation, then I don't think it follows our style guide: http://wiki.blender.org/index.php/Dev:Doc/Code_Style#Indentation

source/gameengine/Expressions/intern/ListWrapper.cpp
199 ↗(On Diff #5199)

This error seems a little misleading. In this case, it is not so much that CListWrapper doesn't support item assignment, but that this instance does not.

237 ↗(On Diff #5199)

Similar to the earlier comment: the class can support access by key, but this instance doesn't support it.

Also, "acces" -> "access"

This revision now requires changes to proceed.Oct 15 2015, 6:41 PM
Porteries Tristan (panzergame) edited edge metadata.
  • Fix wrong indentation in CListWrapper initilization.
  • Fix error messages.
  • Replace void *client by void *self_v
  • Implement get function
  • Rename item to items in error messages
  • Python support for mapping_ass_subscript
Mitchell Stokes (moguri) edited edge metadata.

Some error message tweaks, but otherwise looks good to me.

source/gameengine/Expressions/intern/ListWrapper.cpp
180 ↗(On Diff #5204)

We don't need to mention Python here. We can just have "CListWrapper[i]: List index out of range".

214 ↗(On Diff #5204)

Same as above.

268 ↗(On Diff #5204)

"items type" -> "item type"

278 ↗(On Diff #5204)

"items type" -> "item type"

310 ↗(On Diff #5204)

"items type" -> "item type"

402 ↗(On Diff #5204)

"items type" -> "item type"

Porteries Tristan (panzergame) edited edge metadata.
  • Change "items [type]" to "item type"
source/gameengine/Expressions/EXP_ListWrapper.h
101 ↗(On Diff #5204)

as or ass?

source/gameengine/Expressions/EXP_ListWrapper.h
101 ↗(On Diff #5204)

_ass_ is short for _assign_ here, its convention in Py code, CPython its self calls it mp_ass_subscript.

Since we have some quite long names. eg pyrna_prop_collection_ass_subscript_int, we've kept that convention.

@Sergey Sharybin (sergey), do you still have any issues with this revision?

Sergey Sharybin (sergey) edited edge metadata.

No, not really.

This revision is now accepted and ready to land.Oct 26 2015, 2:11 PM
Angus Hollands (agoose77) edited edge metadata.

Sounds good.

Porteries Tristan (panzergame) edited edge metadata.
  • Allow Setter callback to raise an error in case of wrong item conversion.
This revision was automatically updated to reflect the committed changes.