From 6a270ecb9435fd695988de5c685c65cac5a43c79 Mon Sep 17 00:00:00 2001 From: Campbell Barton Date: Thu, 23 Apr 2009 00:32:33 +0000 Subject: [PATCH] BGE Python API CListValue fixes - Disable changing CValueLists that the BGE uses internally (scene.objects.append(1) would crash when drawing) - val=clist+list would modify clist in place, now return a new value. - clist.append([....]), was working like extend. - clist.append(val) didnt work for most CValue types like KX_GameObjects. Other changes - "isValid" was always returning True. - Set all errors for invalid proxy access to PyExc_SystemError (was using a mix of error types) - Added PyObjectPlus::InvalidateProxy() to manually invalidate, though if python ever gains access again, it will make a new valid proxy. This is so removing an object from a scene can invalidate the object even if its stored elsewhere in a CValueList for eg. --- source/gameengine/Expressions/ListValue.cpp | 153 ++++++++++-------- .../gameengine/Expressions/PyObjectPlus.cpp | 25 ++- source/gameengine/Expressions/PyObjectPlus.h | 2 + source/gameengine/Expressions/Value.cpp | 28 ++-- source/gameengine/Expressions/Value.h | 2 +- source/gameengine/Ketsji/KX_GameObject.cpp | 10 +- source/gameengine/Ketsji/KX_MeshProxy.cpp | 2 +- source/gameengine/Ketsji/KX_Scene.cpp | 10 +- source/gameengine/Ketsji/KX_SceneActuator.cpp | 4 +- 9 files changed, 146 insertions(+), 90 deletions(-) diff --git a/source/gameengine/Expressions/ListValue.cpp b/source/gameengine/Expressions/ListValue.cpp index 6c92e805745..c78963142d1 100644 --- a/source/gameengine/Expressions/ListValue.cpp +++ b/source/gameengine/Expressions/ListValue.cpp @@ -39,8 +39,10 @@ Py_ssize_t listvalue_bufferlen(PyObject* self) PyObject* listvalue_buffer_item(PyObject* self, Py_ssize_t index) { CListValue *list= static_cast(BGE_PROXY_REF(self)); + CValue *cval; + if (list==NULL) { - PyErr_SetString(PyExc_IndexError, BGE_PROXY_ERROR_MSG); + PyErr_SetString(PyExc_SystemError, "val = CList[i], "BGE_PROXY_ERROR_MSG); return NULL; } @@ -49,24 +51,25 @@ PyObject* listvalue_buffer_item(PyObject* self, Py_ssize_t index) if (index < 0) index = count+index; - if (index >= 0 && index < count) - { - PyObject* pyobj = list->GetValue(index)->ConvertValueToPython(); - if (pyobj) - return pyobj; - else - return list->GetValue(index)->GetProxy(); - + if (index < 0 || index >= count) { + PyErr_SetString(PyExc_IndexError, "CList[i]: Python ListIndex out of range in CValueList"); + return NULL; } - PyErr_SetString(PyExc_IndexError, "list[i]: Python ListIndex out of range in CValueList"); - return NULL; + + cval= list->GetValue(index); + + PyObject* pyobj = cval->ConvertValueToPython(); + if (pyobj) + return pyobj; + else + return cval->GetProxy(); } PyObject* listvalue_mapping_subscript(PyObject* self, PyObject* pyindex) { CListValue *list= static_cast(BGE_PROXY_REF(self)); if (list==NULL) { - PyErr_SetString(PyExc_IndexError, BGE_PROXY_ERROR_MSG); + PyErr_SetString(PyExc_SystemError, "value = CList[i], "BGE_PROXY_ERROR_MSG); return NULL; } @@ -85,7 +88,7 @@ PyObject* listvalue_mapping_subscript(PyObject* self, PyObject* pyindex) } PyObject *pyindex_str = PyObject_Repr(pyindex); /* new ref */ - PyErr_Format(PyExc_KeyError, "list[key]: '%s' key not in list", PyString_AsString(pyindex_str)); + PyErr_Format(PyExc_KeyError, "CList[key]: '%s' key not in list", PyString_AsString(pyindex_str)); Py_DECREF(pyindex_str); return NULL; } @@ -96,7 +99,7 @@ PyObject* listvalue_buffer_slice(PyObject* self,Py_ssize_t ilow, Py_ssize_t ihig { CListValue *list= static_cast(BGE_PROXY_REF(self)); if (list==NULL) { - PyErr_SetString(PyExc_IndexError, BGE_PROXY_ERROR_MSG); + PyErr_SetString(PyExc_SystemError, "val = CList[i:j], "BGE_PROXY_ERROR_MSG); return NULL; } @@ -127,73 +130,79 @@ PyObject* listvalue_buffer_slice(PyObject* self,Py_ssize_t ilow, Py_ssize_t ihig } - -static PyObject * -listvalue_buffer_concat(PyObject * self, PyObject * other) +/* clist + list, return a list that python owns */ +static PyObject *listvalue_buffer_concat(PyObject * self, PyObject * other) { CListValue *listval= static_cast(BGE_PROXY_REF(self)); + int i, numitems, numitems_orig; + if (listval==NULL) { - PyErr_SetString(PyExc_IndexError, BGE_PROXY_ERROR_MSG); + PyErr_SetString(PyExc_SystemError, "CList+other, "BGE_PROXY_ERROR_MSG); return NULL; } + numitems_orig= listval->GetCount(); + // for now, we support CListValue concatenated with items // and CListValue concatenated to Python Lists // and CListValue concatenated with another CListValue - listval->AddRef(); - if (other->ob_type == &PyList_Type) + /* Shallow copy, dont use listval->GetReplica(), it will screw up with KX_GameObjects */ + CListValue* listval_new = new CListValue(); + + if (PyList_Check(other)) { + CValue* listitemval; bool error = false; - - int i; - int numitems = PyList_Size(other); + + numitems = PyList_Size(other); + + /* copy the first part of the list */ + listval_new->Resize(numitems_orig + numitems); + for (i=0;iSetValue(i, listval->GetValue(i)->AddRef()); + for (i=0;iConvertPythonToValue(listitem); - if (listitemval) - { - listval->Add(listitemval); - } else - { - error = true; + listitemval = listval->ConvertPythonToValue(PyList_GetItem(other,i), "cList + pyList: CListValue, "); + + if (listitemval) { + listval_new->SetValue(i+numitems_orig, listitemval); + } else { + error= true; + break; } } - + if (error) { - PyErr_SetString(PyExc_SystemError, "list.append(val): couldn't add one or more items to this CValueList"); + listval_new->Resize(numitems_orig+i); /* resize so we dont try release NULL pointers */ + listval_new->Release(); + return NULL; /* ConvertPythonToValue above sets the error */ + } + + } + else if (PyObject_TypeCheck(other, &CListValue::Type)) { + // add items from otherlist to this list + CListValue* otherval = static_cast(BGE_PROXY_REF(other)); + if(otherval==NULL) { + listval_new->Release(); + PyErr_SetString(PyExc_SystemError, "CList+other, "BGE_PROXY_ERROR_MSG); return NULL; } - - } else - { - if (other->ob_type == &CListValue::Type) - { - // add items from otherlist to this list - CListValue* otherval = (CListValue*) other; - - - for (int i=0;iGetCount();i++) - { - otherval->Add(listval->GetValue(i)->AddRef()); - } - } - else - { - CValue* objval = listval->ConvertPythonToValue(other); - if (objval) - { - listval->Add(objval); - } else - { - PyErr_SetString(PyExc_SystemError, "list.append(i): couldn't add item to this CValueList"); - return NULL; - } - } + + numitems = otherval->GetCount(); + + /* copy the first part of the list */ + listval_new->Resize(numitems_orig + numitems); /* resize so we dont try release NULL pointers */ + for (i=0;iSetValue(i, listval->GetValue(i)->AddRef()); + + /* now copy the other part of the list */ + for (i=0;iSetValue(i+numitems_orig, otherval->GetValue(i)->AddRef()); + } - - return self; + return listval_new->NewProxy(true); /* python owns this list */ } @@ -437,14 +446,24 @@ void CListValue::MergeList(CListValue *otherlist) { SetValue(i+numelements,otherlist->GetValue(i)->AddRef()); } - } - PyObject* CListValue::Pyappend(PyObject* value) { - return listvalue_buffer_concat(m_proxy, value); /* m_proxy is the same as self */ + CValue* objval = ConvertPythonToValue(value, "CList.append(i): CValueList, "); + + if (!objval) /* ConvertPythonToValue sets the error */ + return NULL; + + if (!BGE_PROXY_PYOWNS(m_proxy)) { + PyErr_SetString(PyExc_TypeError, "CList.append(i): this CValueList is used internally for the game engine and can't be modified"); + return NULL; + } + + Add(objval); + + Py_RETURN_NONE; } @@ -482,7 +501,7 @@ PyObject* CListValue::Pyindex(PyObject *value) { PyObject* result = NULL; - CValue* checkobj = ConvertPythonToValue(value); + CValue* checkobj = ConvertPythonToValue(value, "val = cList[i]: CValueList, "); if (checkobj==NULL) return NULL; /* ConvertPythonToValue sets the error */ @@ -499,7 +518,7 @@ PyObject* CListValue::Pyindex(PyObject *value) checkobj->Release(); if (result==NULL) { - PyErr_SetString(PyExc_ValueError, "list.index(x): x not in CListValue"); + PyErr_SetString(PyExc_ValueError, "CList.index(x): x not in CListValue"); } return result; @@ -511,7 +530,7 @@ PyObject* CListValue::Pycount(PyObject* value) { int numfound = 0; - CValue* checkobj = ConvertPythonToValue(value); + CValue* checkobj = ConvertPythonToValue(value, "cList.count(val): CValueList, "); if (checkobj==NULL) { /* in this case just return that there are no items in the list */ PyErr_Clear(); diff --git a/source/gameengine/Expressions/PyObjectPlus.cpp b/source/gameengine/Expressions/PyObjectPlus.cpp index 57a61ac37ae..54f076741cc 100644 --- a/source/gameengine/Expressions/PyObjectPlus.cpp +++ b/source/gameengine/Expressions/PyObjectPlus.cpp @@ -138,9 +138,9 @@ PyObject *PyObjectPlus::py_base_getattro(PyObject * self, PyObject *attr) PyObjectPlus *self_plus= BGE_PROXY_REF(self); if(self_plus==NULL) { if(!strcmp("isValid", PyString_AsString(attr))) { - Py_RETURN_TRUE; + Py_RETURN_FALSE; } - PyErr_SetString(PyExc_RuntimeError, BGE_PROXY_ERROR_MSG); + PyErr_SetString(PyExc_SystemError, BGE_PROXY_ERROR_MSG); return NULL; } @@ -171,7 +171,7 @@ int PyObjectPlus::py_base_setattro(PyObject *self, PyObject *attr, PyObject *val { PyObjectPlus *self_plus= BGE_PROXY_REF(self); if(self_plus==NULL) { - PyErr_SetString(PyExc_RuntimeError, BGE_PROXY_ERROR_MSG); + PyErr_SetString(PyExc_SystemError, BGE_PROXY_ERROR_MSG); return -1; } @@ -186,7 +186,7 @@ PyObject *PyObjectPlus::py_base_repr(PyObject *self) // This should be the ent PyObjectPlus *self_plus= BGE_PROXY_REF(self); if(self_plus==NULL) { - PyErr_SetString(PyExc_RuntimeError, BGE_PROXY_ERROR_MSG); + PyErr_SetString(PyExc_SystemError, BGE_PROXY_ERROR_MSG); return NULL; } @@ -818,6 +818,23 @@ void PyObjectPlus::ProcessReplica() m_proxy= NULL; } +/* Sometimes we might want to manually invalidate a BGE type even if + * it hasnt been released by the BGE, say for example when an object + * is removed from a scene, accessing it may cause problems. + * + * In this case the current proxy is made invalid, disowned, + * and will raise an error on access. However if python can get access + * to this class again it will make a new proxy and work as expected. + */ +void PyObjectPlus::InvalidateProxy() // check typename of each parent +{ + if(m_proxy) { + BGE_PROXY_REF(m_proxy)=NULL; + Py_DECREF(m_proxy); + m_proxy= NULL; + } +} + /* Utility function called by the macro py_getattro_up() * for getting ob.__dict__() values from our PyObject * this is used by python for doing dir() on an object, so its good diff --git a/source/gameengine/Expressions/PyObjectPlus.h b/source/gameengine/Expressions/PyObjectPlus.h index 52e59f7730c..257851ef4b3 100644 --- a/source/gameengine/Expressions/PyObjectPlus.h +++ b/source/gameengine/Expressions/PyObjectPlus.h @@ -451,6 +451,8 @@ public: static PyObject *GetProxy_Ext(PyObjectPlus *self, PyTypeObject *tp); static PyObject *NewProxy_Ext(PyObjectPlus *self, PyTypeObject *tp, bool py_owns); + void InvalidateProxy(); + /** * Makes sure any internal data owned by this class is deep copied. */ diff --git a/source/gameengine/Expressions/Value.cpp b/source/gameengine/Expressions/Value.cpp index b86fbe0b163..6e8f2ba1061 100644 --- a/source/gameengine/Expressions/Value.cpp +++ b/source/gameengine/Expressions/Value.cpp @@ -604,7 +604,7 @@ PyObject* CValue::py_getattro_dict() { py_getattro_dict_up(PyObjectPlus); } -CValue* CValue::ConvertPythonToValue(PyObject* pyobj) +CValue* CValue::ConvertPythonToValue(PyObject* pyobj, const char *error_prefix) { CValue* vallie = NULL; @@ -620,7 +620,7 @@ CValue* CValue::ConvertPythonToValue(PyObject* pyobj) for (i=0;iAdd(listitemval); @@ -657,13 +657,22 @@ CValue* CValue::ConvertPythonToValue(PyObject* pyobj) { vallie = new CStringValue(PyString_AsString(pyobj),""); } else - if (pyobj->ob_type==&CValue::Type || pyobj->ob_type==&CListValue::Type) + if (BGE_PROXY_CHECK_TYPE(pyobj)) /* Note, dont let these get assigned to GameObject props, must check elsewhere */ { - vallie = ((CValue*) pyobj)->AddRef(); + if (BGE_PROXY_REF(pyobj) && (BGE_PROXY_REF(pyobj))->isA(&CValue::Type)) + { + vallie = (static_cast(BGE_PROXY_REF(pyobj)))->AddRef(); + } else { + + if(BGE_PROXY_REF(pyobj)) /* this is not a CValue */ + PyErr_Format(PyExc_TypeError, "%sgame engine python type cannot be used as a property", error_prefix); + else /* PyObjectPlus_Proxy has been removed, cant use */ + PyErr_Format(PyExc_SystemError, "%s"BGE_PROXY_ERROR_MSG, error_prefix); + } } else { /* return an error value from the caller */ - PyErr_SetString(PyExc_TypeError, "This python type could not be converted to a to a game engine property"); + PyErr_Format(PyExc_TypeError, "%scould convert python value to a game engine property", error_prefix); } return vallie; @@ -682,10 +691,11 @@ int CValue::py_delattro(PyObject *attr) int CValue::py_setattro(PyObject *attr, PyObject* pyobj) { char *attr_str= PyString_AsString(attr); - CValue* oldprop = GetProperty(attr_str); - - CValue* vallie = ConvertPythonToValue(pyobj); - if (vallie) + CValue* oldprop = GetProperty(attr_str); + CValue* vallie; + + /* Dissallow python to assign GameObjects, Scenes etc as values */ + if ((BGE_PROXY_CHECK_TYPE(pyobj)==0) && (vallie = ConvertPythonToValue(pyobj, "cvalue.attr = value: "))) { if (oldprop) oldprop->SetValue(vallie); diff --git a/source/gameengine/Expressions/Value.h b/source/gameengine/Expressions/Value.h index 065fe62d978..97ce9cddcea 100644 --- a/source/gameengine/Expressions/Value.h +++ b/source/gameengine/Expressions/Value.h @@ -230,7 +230,7 @@ public: return NULL; } - virtual CValue* ConvertPythonToValue(PyObject* pyobj); + virtual CValue* ConvertPythonToValue(PyObject* pyobj, const char *error_prefix); virtual int py_delattro(PyObject *attr); diff --git a/source/gameengine/Ketsji/KX_GameObject.cpp b/source/gameengine/Ketsji/KX_GameObject.cpp index bf6a81c8493..7629f9d8f1a 100644 --- a/source/gameengine/Ketsji/KX_GameObject.cpp +++ b/source/gameengine/Ketsji/KX_GameObject.cpp @@ -1200,7 +1200,7 @@ PyObject *KX_GameObject::Map_GetItem(PyObject *self_v, PyObject *item) PyObject* pyconvert; if (self==NULL) { - PyErr_SetString(PyExc_RuntimeError, BGE_PROXY_ERROR_MSG); + PyErr_SetString(PyExc_SystemError, BGE_PROXY_ERROR_MSG); return NULL; } @@ -1234,7 +1234,7 @@ int KX_GameObject::Map_SetItem(PyObject *self_v, PyObject *key, PyObject *val) PyErr_Clear(); if (self==NULL) { - PyErr_SetString(PyExc_RuntimeError, BGE_PROXY_ERROR_MSG); + PyErr_SetString(PyExc_SystemError, BGE_PROXY_ERROR_MSG); return -1; } @@ -1261,9 +1261,9 @@ int KX_GameObject::Map_SetItem(PyObject *self_v, PyObject *key, PyObject *val) int set= 0; /* as CValue */ - if(attr_str) + if(attr_str && BGE_PROXY_CHECK_TYPE(val)==0) /* dont allow GameObjects for eg to be assigned to CValue props */ { - CValue* vallie = self->ConvertPythonToValue(val); + CValue* vallie = self->ConvertPythonToValue(val, ""); /* error unused */ if(vallie) { @@ -2643,7 +2643,7 @@ bool ConvertPythonToGameObject(PyObject * value, KX_GameObject **object, bool py /* sets the error */ if (*object==NULL) { - PyErr_Format(PyExc_RuntimeError, "%s, " BGE_PROXY_ERROR_MSG, error_prefix); + PyErr_Format(PyExc_SystemError, "%s, " BGE_PROXY_ERROR_MSG, error_prefix); return false; } diff --git a/source/gameengine/Ketsji/KX_MeshProxy.cpp b/source/gameengine/Ketsji/KX_MeshProxy.cpp index 75e46da072e..009364a3d7b 100644 --- a/source/gameengine/Ketsji/KX_MeshProxy.cpp +++ b/source/gameengine/Ketsji/KX_MeshProxy.cpp @@ -348,7 +348,7 @@ bool ConvertPythonToMesh(PyObject * value, RAS_MeshObject **object, bool py_none /* sets the error */ if (*object==NULL) { - PyErr_Format(PyExc_RuntimeError, "%s, " BGE_PROXY_ERROR_MSG, error_prefix); + PyErr_Format(PyExc_SystemError, "%s, " BGE_PROXY_ERROR_MSG, error_prefix); return false; } diff --git a/source/gameengine/Ketsji/KX_Scene.cpp b/source/gameengine/Ketsji/KX_Scene.cpp index 6917305522e..0c26a6a7b3b 100644 --- a/source/gameengine/Ketsji/KX_Scene.cpp +++ b/source/gameengine/Ketsji/KX_Scene.cpp @@ -891,7 +891,15 @@ void KX_Scene::RemoveObject(class CValue* gameobj) { KX_GameObject* newobj = (KX_GameObject*) gameobj; - // first disconnect child from parent + /* Invalidate the python reference, since the object may exist in script lists + * its possible that it wont be automatically invalidated, so do it manually here, + * + * if for some reason the object is added back into the scene python can always get a new Proxy + */ + gameobj->InvalidateProxy(); + + + // disconnect child from parent SG_Node* node = newobj->GetSGNode(); if (node) diff --git a/source/gameengine/Ketsji/KX_SceneActuator.cpp b/source/gameengine/Ketsji/KX_SceneActuator.cpp index 6f622939dc4..7d2bea25d2c 100644 --- a/source/gameengine/Ketsji/KX_SceneActuator.cpp +++ b/source/gameengine/Ketsji/KX_SceneActuator.cpp @@ -316,7 +316,7 @@ int KX_SceneActuator::pyattr_set_camera(void *self, const struct KX_PYATTRIBUTE_ if(camOb==NULL) { - PyErr_SetString(PyExc_RuntimeError, BGE_PROXY_ERROR_MSG); + PyErr_SetString(PyExc_SystemError, BGE_PROXY_ERROR_MSG); return 1; } @@ -435,7 +435,7 @@ PyObject* KX_SceneActuator::PySetCamera(PyObject* args) new_camera = static_castBGE_PROXY_REF(cam); if(new_camera==NULL) { - PyErr_SetString(PyExc_RuntimeError, BGE_PROXY_ERROR_MSG); + PyErr_SetString(PyExc_SystemError, BGE_PROXY_ERROR_MSG); return NULL; }