Page MenuHome

BGE : Standardization of callbacks execution.
ClosedPublic

Authored by Porteries Tristan (panzergame) on Feb 12 2015, 8:26 PM.

Details

Summary

A new function (RunPythonCallBackList) to call all python functions contained in a python list was developed.

This function has:

  • first argument is the python list of callbacks
  • second argument is a python list of arguments
  • third argument is the minimum quantity of arguments
  • forth argument is the maximum quantity of arguments

It improves flexibility and support the *args.

Diff Detail

Repository
rB Blender

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Note, I think if this patch is to be applied we should make sure it can work with 2+ different kinds of callback lists.

Its not very useful to have a generic callback list if we can only use it in one place.

Porteries Tristan (panzergame) edited edge metadata.

Use this list for pre_draw and post_draw callback list from KX_Scene.
Fix license blocks.

The added benefit of the patch seems to be an easy to use way to call callbacks with a variable number of arguments. However, I see a few things that should be discussed:

  • All callbacks are handled in the same way. The maximum number of supported parameters is passed along, depending on co_argcount. This doesn't support the current collision callback scenario, as there we pass either one or three arguments, but never two.
  • The above also means that it becomes impossible to create a callback that takes *args, as in that case co_argcount=0 and no arguments will be passed at all.

I would also like to see some example Python code, to see what it looks like from that side of things. Right now the callbacks are pretty easy to use. Does the introduction of a FunctionValue also require a Python programmer to use that type? Or will it be transparent?

Concerning code style: the code contains a lot of these types of structures:

if (is_ok1) {
   if (is_ok2) {
      ... perform core functionality
   }
   else {
      ... handle error 2
}
else {
   ... handle error 1
}

The result is that the check for condition X is relatively far away from the error handling of that condition. Doing this fixes that:

if (is_bad1) {
   ... handle error 1
   return ERROR_VALUE;
}

if (is_bad2) {
   ... handle error 2
   return ERROR_VALUE;
}

... perform core functionality

It also has the added benefit that an extra condition only adds a few lines, and doesn't indent the rest of the code (and thus Git also records only the added condition). As an example, I would write the FindOrAddArgsTuple method like this:

PyObject *CFunctionListValue::FindOrAddArgsTuple(unsigned short argcount, PyObject *firstarg, va_list arglist)
{
    PyObject *tuple;
    
    if (argcount == 0)
        return NULL;

    if (m_argCountToTuples[argcount])
        return m_argCountToTuples[argcount];

    tuple = PyTuple_New(argcount);
    for (unsigned short j = 0; j < argcount; ++j) {
        if (j == 0)
            PyTuple_SET_ITEM(tuple, j, firstarg);
        else
            PyTuple_SET_ITEM(tuple, j, va_arg(arglist, PyObject*));
    }

    m_argCountToTuples[argcount] = tuple;
    return tuple;
}

The methods & classes should also be documented more, there's hardly any comment (and part of it is French). I know the current BGE code also isn't a great example of well-documented code, so let's head into the right direction when we add new stuff.

source/gameengine/Expressions/FunctionListValue.cpp
83 ↗(On Diff #3583)

CFunctionValue::Execute can return NULL, so this may crash.

source/gameengine/Expressions/ListValue.h
65 ↗(On Diff #3583)

Naming: I would expect a function with this name to set a Python modifier (whatever that may be). However, this sets a boolean that reflects whether this list can be modified from Python. SetPythonModificationAllowed would be more suitable. Alternatively we could even flip the meaning, and make it a "mutable" flag, which would be more consistent with Python terminology.

To me it's unclear when and why the list may NOT be mutable though. Are there lists currently in use in the BGE that need such protection?

source/gameengine/Ketsji/KX_Scene.cpp
2110

Why do a check cb_list->GetCount() > 0? Can't this be moved to cb_list->ExecuteAllFunctions() instead?

2392

This (almost) identical code is used at every setter. It may be better to reduce the amount of boilerplate code and move it into its own method/function.

source/gameengine/Ketsji/KX_Scene.h
635 ↗(On Diff #3583)

No idea. A comment in the code may also not be the best place to get an answer ;-)

Fix condition code style.
Add documentations (sorry for my horrible English).
Rename SetPythonModifier to SetPythonModificationAllowed.
Check return of Execute function and empty list in ExecuteAllFunctions.

@Porteries Tristan (panzergame) could you give me some more information wrt my questions?

First Question : I think that is possible to make a collision callback which only takes the object in collision and the point, but yes will be never 0 arguments (minimal arguments like maximal arguments variable).

Second Question : I don't know how to fix that, it's a big problem which exists before and now.

I would also like to see some example Python code, to see what it looks like from that side of things. Right now the callbacks are pretty easy to use. Does the introduction of a FunctionValue also require a Python programmer to use that type? Or will it be transparent?

obj = objList["Cube"]

def onCollision(collider, point, normal):
    print("collider :", collider, ", point :", point, ", normal :", normal)

obj.collisionCallbacks.append(onCollision) # append the function
obj.collisionCallbacks.pop(0) # pop the first elements
obj.collisionCallbacks = [onCollision] # list assignement
obj.collisionCallbacks = ["error value"] # error

Yes FunctionValue is transparent it inherite of CValue to use CListValue
I know there are a crash with two functions i'm working on.

Defining a new PyType and CFunctionListValue is overkill (imho).

Wouldn't a PyList of tuples should do just as well here? (and a lot less code).

The way the CFunctionValue is named. it looks like a generic callback, but is infact Python specific (which is misleading) and looks like its not trivial to extend to non Python callbacks.

Would prefer not to bother with a new C++ class, and just use utility functions for handling Python arg-lists, which can be re-used, the same way this class intends.

Campbell Barton (campbellbarton) requested changes to this revision.Feb 28 2015, 2:45 AM
Campbell Barton (campbellbarton) edited edge metadata.
This revision now requires changes to proceed.Feb 28 2015, 2:45 AM
Porteries Tristan (panzergame) edited edge metadata.
  • Remove useless class CFunctionValue so CFunctionValueList deosn't inherite of CListValue and just contains a python list of functions.
  • To access to the python list of a CFuctionListValue : GetFunctionList() and to set : SetFunctionList(list) because of reference counting.
  • Correct tuple creation by copying va_list to preserve the original.

example python code :

obj = objList["Cube0"]

def onCollision(collider, point, normal):
    print("collider :", collider, ", point :", point, ", normal :", normal)

def onCollisionNoNormal(collider, point):
    print("collider :", collider, ", point :", point)

def onCollisionOnlyCollider(collider):
    print("collider :", collider)

def onCollisionNoArgs():
    pass

def drawCall():
    print(sce)

def drawCallOneArgument(arg):
    print(arg)

class gameObject:
    def onCollision(self, collider, point, normal):
        print("this :", id(self), "collider :", collider, ", point :", point, ", normal :", normal)

# we can use collisionCallbacks like a python list
obj.collisionCallbacks.append(onCollisionNoNormal)
obj.collisionCallbacks.append(onCollision)
obj.collisionCallbacks.append(onCollisionOnlyCollider)
gobj = gameObject()
print(id(gobj))
obj.collisionCallbacks.append(gobj.onCollision)
obj.collisionCallbacks.append(onCollisionNoArgs) # error
sce.pre_draw.append(drawCall)
sce.pre_draw.append(drawCallOneArgument) # error
Porteries Tristan (panzergame) edited edge metadata.

Cleanup blender code style.

source/gameengine/Expressions/FunctionListValue.cpp
43 ↗(On Diff #3677)

why have firstarg? - this means having zero args is a bit confusing.

50–51 ↗(On Diff #3677)

Why is this needed?

62–63 ↗(On Diff #3677)

Don't think this is the case. At the end of this function the tuple will have a user-count of 2. Which isnt right since its only used in one place.

If there are reference counting issues, they should be resolved elsewhere.

131–135 ↗(On Diff #3677)

If this is NULL checking already. you can use else { Py_DECREF(ret); }

source/gameengine/Expressions/FunctionListValue.h
36 ↗(On Diff #3677)

Still not so happy with using a class here. All of this can just be function calls.

Only 3?

  • Ensure arg tuple is created.
  • Call callbacks
  • Free arg tuples.

Really think that this is a lot clearer to have as functions and theres no need to do OOP.

50 ↗(On Diff #3677)

no need to use a map. Caller can just use PyObject *args[maxArgCount] = {NULL};

source/gameengine/Expressions/FunctionListValue.cpp
43 ↗(On Diff #3677)

For use va_list we have to specify the type of the first element but the first element is not in the va_list, so we have to pass the first element and others contain in the va_list.

50–51 ↗(On Diff #3677)

the function va_arg converts and REMOVES each elements of the va_list past and seems to be a pointer so we have to copy the va_list otherwise we get a segfault.

source/gameengine/Expressions/FunctionListValue.h
36 ↗(On Diff #3677)

If not use an object we have to pass to the execute function the python list, the minimun/maximum arguments count and the python arguments.
like :

Execute(functionlist, minargs, maxargs, arg1, ...)

i think is more easy to create an object and just call a function with the minimum of arguments possible.

50 ↗(On Diff #3677)

I think it's more optimized to use a map than a array, and i think also is not to the user to allocate the tuple array used internally.

fix r_argcount, sorry for that.

This patch handles Python ref-counting baddly (almost certainly leaks memory, see previous comment re: refcounting),

To me this can just be some simple code de-duplication and no need to add OOP, this may be my bias from not doing much C++, but keep in mind you're writing to CPython here which is a C API too and its typical to using basic C constructs when writing to it,
issues like using std::map for storing a few pointers just seem like general over complication.

Even naming is misleading FunctionListValue/CFunctionListValue - there is nothing todo with CFunctions here - its just a CPython/API helper.

I don't especially want to spend more time reviewing this patch, if you like to investigate making the patch a simple de-dupliction of code - thats fine. Otherwise I would reject it in its current direction.

source/gameengine/Expressions/FunctionListValue.cpp
76–137 ↗(On Diff #3686)

Not really great to have 2x separate code paths for executing callbacks.

Campbell Barton (campbellbarton) requested changes to this revision.Mar 6 2015, 4:50 AM
Campbell Barton (campbellbarton) edited edge metadata.
This revision now requires changes to proceed.Mar 6 2015, 4:50 AM
Porteries Tristan (panzergame) edited edge metadata.

Use of a simple function RunPythonCallBackList which take the python list and the minimun/maximun number of arguments, it use internaly others functions :
CheckPythonFunction and CreatePythonTuple (static)

it's what you want campbellbarton ?

Yep, this is much closer... reply inline.

source/gameengine/Expressions/CMakeLists.txt
58

Could call this KX_PythonCallback.cpp/h ? - since its specifically python. (KX/SCA... is a bit of a mess currently)

source/gameengine/Expressions/CallBacks.cpp
55 ↗(On Diff #3697)

not convinced varargs is great here. especially since it needs to be copied...

PyObject *args[] = {a, b, c};
RunCallbackList(list, args, argcount_min, ARRAY_SIZE(args));

Works too and avoids firstarg/varargs overhead.

57–62 ↗(On Diff #3697)

This is going to leak memory. Since the PyObject arguments are created, then never used/freed.

source/gameengine/Ketsji/KX_GameObject.cpp
1579

Probably best only to call this function, if the list is nonzero length, that way you can avoid creating PyObject's only to free immediately after.

Rename CallBack to KX_PythonCallBack and use of python object array for arguments.

This is much closer, noted some issues/improvements to make.

source/gameengine/Expressions/KX_PythonCallBack.cpp
48

Can be:

    argTuples[(maxargcount - minargcount) + 1];


Then offset the index:

    argTuples[funcargcount - minargcount];
89

This is going to leak memory. (creating and object and not freeing it), replace %s with %R.

However printing an entire object is a bit risky (could be anything... pages of data), best just print the type, eg from Color_subscript:

		PyErr_Format(PyExc_TypeError,
		             "color indices must be integers, not %.200s",
		             Py_TYPE(item)->tp_name);

Use of the item name wherease representation and allocate the minimal array for function argument.

One issue I have with changing our Python callbacks to the proposed functionality, is that we lose a lot of callable styles, such as functools.partial, callable instances, variable arguments (*args) etc. For some specific callbacks this might be okay, but for me it's a step too far to do this for all of them.

@Sybren A. Stüvel (sybren) *args should be supported, If the arg count isn't known. it should assume all args are set. (think this is a reasonable, sane default)

Other minor notes.

Would use unsigned int - short's are mainly for when storage is an issue.

source/gameengine/Expressions/KX_PythonCallBack.cpp
76

can be Py_DECREF

source/gameengine/Expressions/KX_PythonCallBack.h
40

This can be a static function (internal use only)

source/gameengine/Ketsji/KX_Scene.cpp
2138–2139

Why replace PyList_GET_SIZE ? (its not error checked, its direct lookup)

source/gameengine/Expressions/KX_PythonCallBack.cpp
76

No, because we don't no if argTuples is totaly filled.

Cleanup short -> int, PyList_Size -> PyList_GET_SIZE, move CheckPythonFunction as static and support for *args.

This revision is now accepted and ready to land.Apr 14 2015, 3:57 PM
Sybren A. Stüvel (sybren) requested changes to this revision.Apr 16 2015, 4:29 PM
Sybren A. Stüvel (sybren) edited edge metadata.

Just a very small idea for another improvement you could squeeze in there.

source/gameengine/Ketsji/KX_Scene.cpp
2138–2139

This check can be moved into RunPythonCallBackList, as then it will transparently handle NULL values and empty lists for every call.

This revision now requires changes to proceed.Apr 16 2015, 4:29 PM
Porteries Tristan (panzergame) edited edge metadata.

newline at end of files

Sybren A. Stüvel (sybren) requested changes to this revision.Apr 16 2015, 4:43 PM
Sybren A. Stüvel (sybren) edited edge metadata.

Another tweak ;-)

source/gameengine/Ketsji/KX_GameObject.cpp
1568–1569

This condition can be inverted to return immediately. This un-indents the rest of the code, and makes the code easier to read:

if (!m_collisionCallbacks || PyList_GET_SIZE(m_collisionCallbacks) == 0) return;
This revision now requires changes to proceed.Apr 16 2015, 4:43 PM
Porteries Tristan (panzergame) edited edge metadata.

Move callback list check at the top.

This revision is now accepted and ready to land.Apr 16 2015, 5:03 PM
This revision was automatically updated to reflect the committed changes.

This change still needs a mention in the release notes and other relevant documentation should be updated. It also needs an example blend file for the release notes.