Merge pull request #59 from maniek2332/ft/node_ptr

Using NodePtr/NodeOwnerPtr, fix for CPyNodeWrapper making a reference cycle

kaa/custom_transitions.pxi

@@ -13,10 +13,10 @@ from .kaacore.custom_transitions cimport (
 
 
 cdef void node_transition_callback_dispatch(
-    const CPythonicCallbackWrapper c_wrapper, CNode* c_node
+    const CPythonicCallbackWrapper c_wrapper, CNodePtr c_node_ptr
 ):
     # TODO weak-ptr detection
-    (<object>c_wrapper.py_callback)(get_node_wrapper(c_node))
+    (<object>c_wrapper.py_callback)(get_node_wrapper(c_node_ptr))
 
 
 @cython.final
@@ -52,19 +52,19 @@ cdef cppclass CPyNodeCustomTransition(CNodeTransitionCustomizable):
         this.internal_duration = duration
         this.warping = warping
 
-    unique_ptr[CTransitionStateBase] prepare_state(CNode* c_node) const:
-        cdef object state_object = (<object>this.prepare_func.py_callback)(get_node_wrapper(c_node))
+    unique_ptr[CTransitionStateBase] prepare_state(CNodePtr c_node_ptr) const:
+        cdef object state_object = (<object>this.prepare_func.py_callback)(get_node_wrapper(c_node_ptr))
         return <unique_ptr[CTransitionStateBase]> \
             unique_ptr[CPyNodeCustomTransitionState](
                 new CPyNodeCustomTransitionState(state_object)
             )
 
-    void evaluate(CTransitionStateBase* state, CNode* c_node, const double t) const:
+    void evaluate(CTransitionStateBase* state, CNodePtr c_node_ptr, const double t) const:
         cdef CPyNodeCustomTransitionState* custom_state = \
             <CPyNodeCustomTransitionState*>state
 
         (<object>this.evaluate_func.py_callback)(custom_state.state_object,
-                                                 get_node_wrapper(c_node), t)
+                                                 get_node_wrapper(c_node_ptr), t)
 
 
 @cython.final

kaa/extra/include/python_exceptions_wrapper.h

@@ -20,7 +20,7 @@ struct PythonException : std::exception {
 
     ~PythonException()
     {
-        KAACORE_ASSERT(PyGILState_Check());
+        KAACORE_ASSERT_TERMINATE(PyGILState_Check());
         Py_DECREF(this->py_exception);
     }
 

kaa/extra/include/pythonic_callback.h

@@ -4,6 +4,7 @@
 
 #include "kaacore/physics.h"
 #include "kaacore/transitions.h"
+#include "kaacore/node_ptr.h"
 #include "kaacore/nodes.h"
 #include "kaacore/timers.h"
 #include "kaacore/input.h"
@@ -113,7 +114,7 @@ TimerCallback bind_cython_timer_callback(
 }
 
 
-typedef void (*CythonNodeTransitionCallback)(const PythonicCallbackWrapper, Node*);
+typedef void (*CythonNodeTransitionCallback)(const PythonicCallbackWrapper, NodePtr);
 
 NodeTransitionCallbackFunc bind_cython_transition_callback(
     const CythonNodeTransitionCallback cy_handler, const PythonicCallbackWrapper& callback

kaa/fonts.pxi

@@ -20,7 +20,7 @@ cdef Font get_font_wrapper(const CFont& c_font):
 
 cdef class TextNode(NodeBase):
     def __init__(self, **options):
-        self._init_new_node(CNodeType.text)
+        self._make_c_node(CNodeType.text)
         super().__init__(**options)
 
     def setup(self, **options):

kaa/kaacore/custom_transitions.pxd

@@ -1,7 +1,7 @@
 from libcpp.memory cimport unique_ptr
 from libcpp.functional cimport function
 
-from .nodes cimport CNode
+from .nodes cimport CNodePtr
 from .transitions cimport CTransitionStateBase, CTransitionWarping
 from .glue cimport CPythonicCallbackWrapper
 from .exceptions cimport raise_py_error
@@ -18,15 +18,15 @@ cdef extern from "kaacore/transitions.h":
         CNodeTransitionCustomizable(const double duration, const CTransitionWarping& warping) \
             except +raise_py_error
 
-        unique_ptr[CTransitionStateBase] prepare_state(CNode* node) const
-        void evaluate(CTransitionStateBase* state, CNode* node, const double t) const
+        unique_ptr[CTransitionStateBase] prepare_state(CNodePtr node) const
+        void evaluate(CTransitionStateBase* state, CNodePtr node, const double t) const
 
-    ctypedef function[void(CNode*)] CNodeTransitionCallbackFunc "kaacore::NodeTransitionCallbackFunc";
+    ctypedef function[void(CNodePtr)] CNodeTransitionCallbackFunc "kaacore::NodeTransitionCallbackFunc";
 
 
 cdef extern from "extra/include/pythonic_callback.h":
     ctypedef void (*CythonNodeTransitionCallback)(
-        const CPythonicCallbackWrapper, CNode*
+        const CPythonicCallbackWrapper, CNodePtr
     )
     CNodeTransitionCallbackFunc bind_cython_transition_callback(
         const CythonNodeTransitionCallback cy_handler,

kaa/kaacore/nodes.pxd

@@ -14,6 +14,23 @@ from .transitions cimport CNodeTransitionHandle
 from .exceptions cimport raise_py_error
 
 
+cdef extern from "kaacore/node_ptr.h" nogil:
+    cdef cppclass CNodePtr "kaacore::NodePtr":
+        CNodePtr()
+        CNodePtr(CNode*)
+        bool operator==(const CNode*)
+        bool operator bool()
+        CNode* get() except +raise_py_error
+        void destroy() except +raise_py_error
+
+    cdef cppclass CNodeOwnerPtr "kaacore::NodeOwnerPtr":
+        CNodeOwnerPtr()
+        bool operator==(const CNode*)
+        bool operator bool()
+        CNode* get() except +raise_py_error
+        void destroy() except +raise_py_error
+
+
 cdef extern from "kaacore/nodes.h" nogil:
     cdef enum CNodeType "kaacore::NodeType":
         basic "kaacore::NodeType::basic",
@@ -23,7 +40,7 @@ cdef extern from "kaacore/nodes.h" nogil:
         text "kaacore::NodeType::text",
 
     cdef cppclass CForeignNodeWrapper "kaacore::ForeignNodeWrapper":
-        pass
+        void on_add_to_parent()
 
     cdef cppclass CNode "kaacore::Node":
         # UNION!
@@ -32,11 +49,9 @@ cdef extern from "kaacore/nodes.h" nogil:
         CHitboxNode hitbox
         CTextNode text
 
-        CNode(CNodeType type) except +raise_py_error
-
         vector[CNode*]& children() except +raise_py_error
 
-        void add_child(CNode* child_node) except +raise_py_error
+        void add_child(CNodeOwnerPtr child_node) except +raise_py_error
         const CNodeType type() except +raise_py_error
 
         CVector position() except +raise_py_error
@@ -81,7 +96,9 @@ cdef extern from "kaacore/nodes.h" nogil:
         void transition(const CNodeTransitionHandle& transition) except +raise_py_error
 
         CScene* scene() except +raise_py_error
-        CNode* parent() except +raise_py_error
+        CNodePtr parent() except +raise_py_error
 
         void setup_wrapper(unique_ptr[CForeignNodeWrapper]&& wrapper)
         CForeignNodeWrapper* wrapper_ptr() except +raise_py_error
+
+    CNodeOwnerPtr c_make_node "kaacore::make_node" (CNodeType) except +raise_py_error

kaa/kaacore/physics.pxd

@@ -4,7 +4,7 @@ from libcpp cimport bool
 
 from .glue cimport CPythonicCallbackWrapper
 from .vectors cimport CVector
-from .nodes cimport CNode
+from .nodes cimport CNode, CNodePtr
 from .exceptions cimport raise_py_error
 
 
@@ -22,11 +22,11 @@ cdef extern from "kaacore/nodes.h" nogil:
 
     cdef cppclass CArbiter "kaacore::Arbiter":
         CCollisionPhase phase
-        CNode* space
+        CNodePtr space
 
     cdef cppclass CCollisionPair "kaacore::CollisionPair":
-        CNode* body_node
-        CNode* hitbox_node
+        CNodePtr body_node
+        CNodePtr hitbox_node
 
     ctypedef function[int(CArbiter, CCollisionPair, CCollisionPair)] \
         CCollisionHandlerFunc "kaacore::CollisionHandlerFunc"

kaa/nodes.pxi

@@ -1,12 +1,16 @@
 from cpython.ref cimport PyObject, Py_XINCREF, Py_XDECREF
 
 from libc.stdint cimport uint32_t
+from libcpp cimport bool
 from libcpp.memory cimport unique_ptr
 
+from cymove cimport cymove as cmove
+
 from .kaacore.shapes cimport CShape
 from .kaacore.sprites cimport CSprite
 from .kaacore.nodes cimport (
-    CNodeType, CNode, CNodeType, CForeignNodeWrapper
+    CNodeType, CNode, CNodePtr, CNodeOwnerPtr, CForeignNodeWrapper,
+    c_make_node,
 )
 from .kaacore.transitions cimport CNodeTransitionHandle
 from .kaacore.math cimport radians, degrees
@@ -15,58 +19,67 @@ from .kaacore.geometry cimport CAlignment
 
 cdef cppclass CPyNodeWrapper(CForeignNodeWrapper):
     PyObject* py_wrapper
+    bool added_to_parent
 
     __init__(PyObject* py_wrapper):
-        Py_XINCREF(py_wrapper)
         this.py_wrapper = py_wrapper
+        this.added_to_parent = False
 
     __dealloc__():
-        Py_XDECREF(this.py_wrapper)
+        if this.added_to_parent:
+            Py_XDECREF(this.py_wrapper)
         this.py_wrapper = NULL
 
+    void on_add_to_parent() nogil:
+        with gil:
+            Py_XINCREF(py_wrapper)
+            this.added_to_parent = True
+
+
 
 cdef class NodeBase:
     cdef:
-        CNode* c_node
-
-    def __cinit__(self):
-        self.c_node = NULL
+        # When node is created by class __init__ this member will be filled,
+        # it's destructor will be called along with class destructor
+        # (destroying underlying cnode if appropriate).
+        # It's not used if Node class is used as a
+        # wrapper for existing node.
+        CNodeOwnerPtr _c_node_owner_ptr
+        CNodePtr _c_node_ptr
 
     def __init__(self, **options):
         self.setup(**options)
 
     cdef inline CNode* _get_c_node(self):
-        assert self.c_node != NULL, \
+        cdef CNode* c_node = self._c_node_ptr.get()
+        assert c_node != NULL, \
             'Operation on uninitialized or deleted Node. Aborting.'
-        return self.c_node
-
-    cdef void _attach_c_node(self, CNode* c_node):
-        assert self.c_node == NULL
-        assert c_node != NULL
-        self.c_node = c_node
+        return c_node
 
-    cdef void _setup_wrapper(self):
-        assert self.c_node != NULL
-        self.c_node.setup_wrapper(
+    cdef void _make_c_node(self, CNodeType type):
+        self._c_node_owner_ptr = cmove(c_make_node(type))
+        self._c_node_ptr = CNodePtr(self._c_node_owner_ptr.get())
+        self._c_node_ptr.get().setup_wrapper(
             unique_ptr[CForeignNodeWrapper](
                 new CPyNodeWrapper(<PyObject*>self)
             )
         )
 
-    cdef void _init_new_node(self, CNodeType type):
-        cdef CNode* c_node = new CNode(type)
-        self._attach_c_node(c_node)
-        self._setup_wrapper()
+    cdef void _attach_c_node(self, CNodePtr c_node_ptr):
+        assert self._c_node_ptr.get() == NULL, "Node is already initialized, cannot attach."
+        assert c_node_ptr, "Cannot atach NULL node."
+        self._c_node_ptr = c_node_ptr
 
     def add_child(self, NodeBase node):
-        assert self.c_node != NULL
-        assert node.c_node != NULL
-        self.c_node.add_child(node.c_node)
+        assert self._c_node_ptr, "Cannot add_child to NULL node."
+        assert node._c_node_ptr, "Cannot add NULL node as child."
+        assert node._c_node_owner_ptr, "Node added as child must be owned node."
+        self._c_node_ptr.get().add_child(node._c_node_owner_ptr)
         return node
 
     def delete(self):
-        assert self.c_node != NULL
-        del self.c_node
+        assert self._c_node_ptr, "Node already deleted."
+        self._c_node_ptr.destroy()
 
     def setup(self, **options):
         if 'position' in options:
@@ -119,10 +132,10 @@ cdef class NodeBase:
     def children(self):
         cdef:
             CNode* c_node
-            vector[CNode*] children_copy = self.c_node.children()
+            vector[CNode*] children_copy = self._get_c_node().children()
 
         for c_node in children_copy:
-            yield get_node_wrapper(c_node)
+            yield get_node_wrapper(CNodePtr(c_node))
 
     @property
     def type(self):
@@ -136,7 +149,7 @@ cdef class NodeBase:
 
     @property
     def parent(self):
-        if self._get_c_node().parent() != NULL:
+        if self._get_c_node().parent():
             return get_node_wrapper(self._get_c_node().parent())
 
     @property
@@ -287,12 +300,13 @@ cdef class NodeBase:
 
 cdef class Node(NodeBase):
     def __init__(self, **options):
-        self._init_new_node(CNodeType.basic)
+        self._make_c_node(CNodeType.basic)
         super().__init__(**options)
 
 
-cdef NodeBase get_node_wrapper(CNode* c_node):
-    assert c_node != NULL
+cdef NodeBase get_node_wrapper(CNodePtr c_node_ptr):
+    cdef CNode* c_node = c_node_ptr.get()
+    assert c_node != NULL, "Cannot make wrapper for NULL node."
     cdef NodeBase py_node
     if c_node.wrapper_ptr() != NULL:
         # TODO typeid assert?
@@ -301,17 +315,17 @@ cdef NodeBase get_node_wrapper(CNode* c_node):
         ).py_wrapper
     elif c_node.type() == CNodeType.space:
         py_node = SpaceNode.__new__(SpaceNode)
-        py_node._attach_c_node(c_node)
+        py_node._attach_c_node(c_node_ptr)
     elif c_node.type() == CNodeType.body:
         py_node = BodyNode.__new__(BodyNode)
-        py_node._attach_c_node(c_node)
+        py_node._attach_c_node(c_node_ptr)
     elif c_node.type() == CNodeType.hitbox:
         py_node = HitboxNode.__new__(HitboxNode)
-        py_node._attach_c_node(c_node)
+        py_node._attach_c_node(c_node_ptr)
     elif c_node.type() == CNodeType.text:
         py_node = TextNode.__new__(TextNode)
-        py_node._attach_c_node(c_node)
+        py_node._attach_c_node(c_node_ptr)
     else:
         py_node = Node.__new__(Node)
-        py_node._attach_c_node(c_node)
+        py_node._attach_c_node(c_node_ptr)
     return py_node