interrogate: Support subclassing C++ objects from Python

This change enables persistent Python wrapper objects for Python subclasses of typed, reference counted C++ objects.  That means that these objects will store a reference to `self` on the C++ object, and interrogate will always return that instead of making a new Python wrapper every time it is returned from C++.

Practically, this means that you could subclass eg. PandaNode, Event, Fog, what have you, and store these in the scene graph - the Python data in the subclass will be retained and Panda will return your subclass when you ask for the object back rather than creating a new wrapper object without your original data.

To do this, Interrogate generates a proxy class inheriting from the C++ type with additional room to store a `self` pointer and a TypeHandle (which is returned by an overridden `get_type()`).  This TypeHandle is automatically created by registering the Python subclass with the typing system.  The proxy class is only used when the constructor detects that it's constructing for a subtype, so that regular uses of the C++ type are not affected by this mechanism.

(The registration with the typing system could use some improvement.  There's no regard for namespacing right now, in particular.  Furthermore, we could move the registration to an `__init_subclass__()` method, with parameters to specify an existing TypeHandle or to customize the type name.)

This creates a reference cycle, which must be cleared somehow.  One way this happens is by overriding `unref()` in the proxy, which checks that if the Python and C++ reference counts are both 1, this must be the circular reference, and then it breaks the cycle.  Note that this will _only_ work if all other Python references have been cleared before the last C++ reference goes away.  For the other case, we need to rely on Python's garbage collector, so these classes also implement tp_traverse and tp_clear.

This commit also therefore re-enables Python garbage collector support (ie. defining `__traverse__()`), which was previously disabled due to the problems caused by multiple Python wrappers referring to the same C++ object.  To avoid these problems, Panda will only cooperate with Python's GC if the C++ reference count is 1, in which case we can trivially prove that the last reference must be from the Python wrapper.  Note that this explains why we need persistent wrappers for traversal to work--if there are multiple Python wrappers pointing to the C++ object, and they are all participating in a reference cycle, the reference count cannot be 1, and cycle detection does not work.  By default, the GC is only enabled for Python subclasses, but a class may define `__traverse__()` in order to opt-in, keeping the previous limitation in mind.

There is a second mechanism introduced by this commit: classes may define a public `__self__` member of type `PyObject *` if they want to use persistent objects even if they aren't being subclassed.  This allows these classes to participate in Python's GC and avoid the situation above.

The cycle detection is implemented for PandaNode's Python tags, but this is very incomplete, since the traversal doesn't recurse into children and it won't work if there is more than one wrapper object that is part of a cycle, since PandaNode by default doesn't use persistent wrappers.  This problem may need more attention later.

Fixes #1410

dtool/src/interrogate/functionRemap.cxx

@@ -450,8 +450,6 @@ get_call_str(const string &container, const vector_string &pexprs) const {
     _parameters[pn]._remap->pass_parameter(call, get_parameter_expr(pn, pexprs));
 
   } else {
-    const char *separator = "";
-
     // If this function is marked as having an extension function, call that
     // instead.
     if (_extension) {
@@ -488,33 +486,41 @@ get_call_str(const string &container, const vector_string &pexprs) const {
       }
     }
     call << "(";
-
-    if (_flags & F_explicit_self) {
-      // Pass on the PyObject * that we stripped off above.
-      call << separator << "self";
-      separator = ", ";
-    }
-    if (_flags & F_explicit_cls) {
-      call << separator << "cls";
-      separator = ", ";
-    }
-
-    size_t pn;
-    size_t num_parameters = pexprs.size();
-
-    for (pn = _first_true_parameter;
-         pn < num_parameters; ++pn) {
-      nassertd(pn < _parameters.size()) break;
-      call << separator;
-      _parameters[pn]._remap->pass_parameter(call, get_parameter_expr(pn, pexprs));
-      separator = ", ";
-    }
+    write_call_args(call, pexprs);
     call << ")";
   }
 
   return call.str();
 }
 
+/**
+ * Writes the arguments to pass to the function.
+ */
+void FunctionRemap::
+write_call_args(std::ostream &call, const vector_string &pexprs) const {
+  const char *separator = "";
+  if (_flags & F_explicit_self) {
+    // Pass on the PyObject * that we stripped off above.
+    call << separator << "self";
+    separator = ", ";
+  }
+  if (_flags & F_explicit_cls) {
+    call << separator << "cls";
+    separator = ", ";
+  }
+
+  size_t pn;
+  size_t num_parameters = pexprs.size();
+
+  for (pn = _first_true_parameter;
+       pn < num_parameters; ++pn) {
+    nassertd(pn < _parameters.size()) break;
+    call << separator;
+    _parameters[pn]._remap->pass_parameter(call, get_parameter_expr(pn, pexprs));
+    separator = ", ";
+  }
+}
+
 /**
  * Returns the minimum number of arguments that needs to be passed to this
  * function.

dtool/src/interrogate/functionRemap.h

@@ -61,6 +61,7 @@ public:
   FunctionWrapperIndex make_wrapper_entry(FunctionIndex function_index);
 
   std::string get_call_str(const std::string &container, const vector_string &pexprs) const;
+  void write_call_args(std::ostream &out, const vector_string &pexprs) const;
 
   int get_min_num_args() const;
   int get_max_num_args() const;

dtool/src/interrogate/interfaceMakerPythonNative.cxx

@@ -1106,6 +1106,170 @@ write_class_details(ostream &out, Object *obj) {
   std::string ClassName = make_safe_name(obj->_itype.get_scoped_name());
   std::string cClassName = obj->_itype.get_true_name();
 
+  CPPType *cpptype = TypeManager::resolve_type(obj->_itype._cpptype);
+  CPPStructType *struct_type = cpptype->as_struct_type();
+
+  // Determine which external imports we will need.
+  std::map<string, CastDetails> details;
+  std::map<string, CastDetails>::iterator di;
+  builder.get_type(TypeManager::unwrap(cpptype), false);
+  get_valid_child_classes(details, cpptype->as_struct_type());
+  for (di = details.begin(); di != details.end(); di++) {
+    // InterrogateType ptype =idb->get_type(di->first);
+    if (di->second._is_legal_py_class && !isExportThisRun(di->second._structType)) {
+      _external_imports.insert(TypeManager::resolve_type(di->second._structType));
+    }
+    // out << "IMPORT_THIS struct Dtool_PyTypedObject Dtool_" <<
+    // make_safe_name(di->second._to_class_name) << ";\n";
+  }
+
+  bool py_subclassable = is_python_subclassable(struct_type);
+  if (py_subclassable) {
+    assert(!obj->_constructors.empty());
+    out << "/**\n";
+    out << " * Proxy class for " << cClassName << "\n";
+    out << " */\n";
+    out << "class DtoolProxy_" << ClassName << " final : public " << cClassName << ", public DtoolProxy {\n";
+    out << "public:\n";
+    // I'd love just to use "using" to inherit the constructors from the base.
+    // However, MSVC seems to have a bug inheriting constructors with default
+    // arguments.
+    if (struct_type->is_default_constructible()) {
+      out << "  DtoolProxy_" << ClassName << "() = default;\n";
+    }
+    for (Function *func : obj->_constructors) {
+      for (FunctionRemap *remap : func->_remaps) {
+        CPPParameterList::Parameters &params = remap->_ftype->_parameters->_parameters;
+        if (params.empty()) {
+          continue;
+        }
+        out << "  DtoolProxy_" << ClassName << "(";
+        for (size_t i = 0; i < params.size(); ++i) {
+          if (i > 0) {
+            out << ", ";
+          }
+          params[i]->_type->output_instance(out, remap->get_parameter_name(i), &parser);
+          if (i > 0 && params[i]->_initializer != nullptr) {
+            out << " = " << *params[i]->_initializer;
+          }
+        }
+        out << ")";
+        if (remap->_ftype->_flags & CPPFunctionType::F_noexcept) {
+          out << " noexcept";
+        }
+        out << " : " << cClassName << "(";
+        for (size_t i = 0; i < params.size(); ++i) {
+          if (i > 0) {
+            out << ", ";
+          }
+          // Determine whether to pass with move semantics or not.
+          CPPType *param_type = params[i]->_type;
+          CPPType::SubType subtype = param_type->get_subtype();
+          while (subtype == CPPDeclaration::ST_typedef) {
+            param_type = param_type->as_typedef_type()->_type;
+            subtype = param_type->get_subtype();
+          }
+          if (subtype == CPPType::ST_pointer ||
+              subtype == CPPType::ST_const ||
+              subtype == CPPType::ST_enum ||
+              subtype == CPPType::ST_simple ||
+              (subtype == CPPType::ST_reference && param_type->as_reference_type()->_value_category == CPPReferenceType::VC_lvalue)) {
+            out << remap->get_parameter_name(i);
+          } else {
+            out << "std::move(" << remap->get_parameter_name(i) << ")";
+          }
+        }
+        out << ") {}\n";
+      }
+    }
+
+    // Move constructor is never listed above, but we do use it.
+    if (struct_type->is_move_constructible()) {
+      out << "  DtoolProxy_" << ClassName << "(" << cClassName << " &&from)";
+      CPPInstance *ctor = struct_type->get_move_constructor();
+      if (ctor != nullptr) {
+        CPPFunctionType *ftype = ctor->_type->as_function_type();
+        if (ftype != nullptr && (ftype->_flags & CPPFunctionType::F_noexcept) != 0) {
+          out << " noexcept";
+        }
+      }
+      out << " : " << cClassName << "(std::move(from)) {}\n";
+    }
+    out << "\n";
+    out << "  virtual bool unref() const override {\n";
+    out << "    if (!(" << cClassName << "::unref())) {\n";
+    out << "      // It was cleaned up by the Python garbage collector.\n";
+    out << "      return false;\n";
+    out << "    }\n";
+    out << "    nassertr(DtoolProxy::_self != nullptr, true);\n";
+    out << "\n";
+    out << "    // If the last reference to the object is the one being held by Python,\n";
+    out << "    // check whether the Python wrapper itself is also at a refcount of 1.\n";
+    out << "    bool result = true;\n";
+    out << "    if (get_ref_count() == 1) {\n";
+    out << "      PyGILState_STATE gstate = PyGILState_Ensure();\n";
+    out << "      int ref_count = Py_REFCNT(DtoolProxy::_self);\n";
+    out << "      assert(ref_count > 0);\n";
+    out << "      if (ref_count == 1) {\n";
+    out << "        // The last reference to the Python wrapper is being held by us.\n";
+    out << "        // Break the reference cycle and allow the object to go away.\n";
+    out << "        if (!" << cClassName << "::unref()) {\n";
+    out << "          PyObject_GC_UnTrack(DtoolProxy::_self);\n";
+    out << "          ((Dtool_PyInstDef *)DtoolProxy::_self)->_memory_rules = false;\n";
+    out << "          Py_CLEAR(DtoolProxy::_self);\n";
+    out << "\n";
+    out << "          // Let the caller destroy the object.\n";
+    out << "          result = false;\n";
+    out << "        }\n";
+    out << "      }\n";
+    //out << "      else {\n";
+    //out << "        // There is still a Python reference.  Make sure it can be cleaned up\n";
+    //out << "        // by the garbage collector when it gets to it.\n";
+    //out << "        if (!PyObject_GC_IsTracked(DtoolProxy::_self)) {\n";
+    //out << "          PyObject_GC_Track(DtoolProxy::_self);\n";
+    //out << "        }\n";
+    //out << "      }\n";
+    out << "      PyGILState_Release(gstate);\n";
+    out << "    }\n";
+    out << "    return result;\n";
+    out << "  }\n";
+    out << "\n";
+    out << "  virtual TypeHandle get_type() const override {\n";
+    out << "    return DtoolProxy::_type;\n";
+    out << "  }\n";
+    out << "\n";
+    out << "  virtual TypeHandle force_init_type() override {\n";
+    out << "    return DtoolProxy::_type;\n";
+    out << "  }\n";
+    out << "};\n";
+    out << "\n";
+
+    out << "static PyObject *Dtool_WrapProxy_" << ClassName << "(void *from_this, PyTypeObject *from_type) {\n";
+    out << "  DtoolProxy_" << ClassName << " *to_this;\n";
+    out << "  if (from_type == nullptr || from_type == &Dtool_" << ClassName << "._PyType) {\n";
+    out << "    to_this = (DtoolProxy_" << ClassName << " *)(" << cClassName << "*)from_this;\n";
+    out << "  }\n";
+    for (di = details.begin(); di != details.end(); di++) {
+      if (di->second._can_downcast && di->second._is_legal_py_class) {
+        out << "  else if (from_type == (PyTypeObject *)Dtool_Ptr_" << make_safe_name(di->second._to_class_name) << ") {\n";
+        out << "    " << di->second._to_class_name << " *other_this = (" << di->second._to_class_name << " *)from_this;\n" ;
+        out << "    to_this = (DtoolProxy_" << ClassName << " *)(" << cClassName << " *)other_this;\n";
+        out << "  }\n";
+      }
+    }
+    out << "  else {\n";
+    out << "    return nullptr;\n";
+    out << "  }\n";
+    out << "  nassertr(to_this->DtoolProxy::_self != nullptr, nullptr);\n";
+    out << "  PyObject *result = Py_NewRef(to_this->DtoolProxy::_self);\n";
+    // Directly call the parent class' unref(), because we know our version
+    // would only find that the Python refcount > 1 and do nothing else
+    out << "  bool nonzero = to_this->" << cClassName << "::unref();\n";
+    out << "  nassertr(nonzero, result);\n";
+    out << "  return result;\n";
+    out << "}\n\n";
+  }
+
   out << "/**\n";
   out << " * Python wrappers for functions of class " << cClassName << "\n" ;
   out << " */\n";
@@ -1149,11 +1313,9 @@ write_class_details(ostream &out, Object *obj) {
     }
   }
 
-  CPPType *cpptype = TypeManager::resolve_type(obj->_itype._cpptype);
-
   // If we have "coercion constructors", write a single wrapper to consolidate
   // those.
-  int has_coerce = has_coerce_constructor(cpptype->as_struct_type());
+  int has_coerce = has_coerce_constructor(struct_type);
   if (has_coerce > 0) {
     write_coerce_constructor(out, obj, true);
     if (has_coerce > 1 && TypeManager::is_reference_count(obj->_itype._cpptype)) {
@@ -1176,20 +1338,6 @@ write_class_details(ostream &out, Object *obj) {
     }
   }
 
-  // Determine which external imports we will need.
-  std::map<string, CastDetails> details;
-  std::map<string, CastDetails>::iterator di;
-  builder.get_type(TypeManager::unwrap(cpptype), false);
-  get_valid_child_classes(details, cpptype->as_struct_type());
-  for (di = details.begin(); di != details.end(); di++) {
-    // InterrogateType ptype =idb->get_type(di->first);
-    if (di->second._is_legal_py_class && !isExportThisRun(di->second._structType)) {
-      _external_imports.insert(TypeManager::resolve_type(di->second._structType));
-    }
-    // out << "IMPORT_THIS struct Dtool_PyTypedObject Dtool_" <<
-    // make_safe_name(di->second._to_class_name) << ";\n";
-  }
-
   // Write support methods to cast from and to pointers of this type.
   {
     out << "static void *Dtool_UpcastInterface_" << ClassName << "(PyObject *self, Dtool_PyTypedObject *requested_type) {\n";
@@ -1241,27 +1389,46 @@ write_class_details(ostream &out, Object *obj) {
     out << "static PyObject *Dtool_Wrap_" << ClassName << "(void *from_this, PyTypeObject *from_type) {\n";
     out << "  " << cClassName << " *to_this;\n";
     out << "  if (from_type == nullptr || from_type == &Dtool_" << ClassName << "._PyType) {\n";
-    out << "    to_this = (" << cClassName << "*)from_this;\n";
+    out << "    to_this = (" << cClassName << " *)from_this;\n";
     out << "  }\n";
     for (di = details.begin(); di != details.end(); di++) {
       if (di->second._can_downcast && di->second._is_legal_py_class) {
         out << "  else if (from_type == (PyTypeObject *)Dtool_Ptr_" << make_safe_name(di->second._to_class_name) << ") {\n";
         out << "    " << di->second._to_class_name << " *other_this = (" << di->second._to_class_name << " *)from_this;\n" ;
-        out << "    to_this = (" << cClassName << "*)other_this;\n";
+        out << "    to_this = (" << cClassName << " *)other_this;\n";
         out << "  }\n";
       }
     }
     out << "  else {\n";
     out << "    return nullptr;\n";
     out << "  }\n";
-    out << "  // Allocate a new Python instance\n";
-    out << "  Dtool_PyInstDef *self = (Dtool_PyInstDef *)PyType_GenericAlloc(&Dtool_" << ClassName << "._PyType, 0);\n";
-    out << "  self->_signature = PY_PANDA_SIGNATURE;\n";
-    out << "  self->_My_Type = &Dtool_" << ClassName << ";\n";
-    out << "  self->_ptr_to_object = to_this;\n";
-    out << "  self->_memory_rules = false;\n";
-    out << "  self->_is_const = false;\n";
-    out << "  return (PyObject *)self;\n";
+    if (has_self_member(struct_type)) {
+      out << "  PyObject *stored_self = to_this->__self__;\n";
+      out << "  if (stored_self == nullptr) {\n";
+      out << "    // Allocate a new Python instance\n";
+      out << "    Dtool_PyInstDef *self = (Dtool_PyInstDef *)PyType_GenericAlloc(&Dtool_" << ClassName << "._PyType, 0);\n";
+      out << "    self->_signature = PY_PANDA_SIGNATURE;\n";
+      out << "    self->_My_Type = &Dtool_" << ClassName << ";\n";
+      out << "    self->_ptr_to_object = to_this;\n";
+      out << "    self->_memory_rules = true;\n";
+      out << "    self->_is_const = false;\n";
+      out << "    to_this->__self__ = Py_NewRef((PyObject *)self);\n";
+      out << "    return Py_NewRef((PyObject *)self);\n";
+      out << "  }\n";
+      out << "  PyObject *result = Py_NewRef(stored_self);\n";
+      out << "  bool nonzero = to_this->unref();\n";
+      out << "  nassertr(nonzero, result);\n";
+      out << "  return result;\n";
+    } else {
+      out << "  // Allocate a new Python instance\n";
+      out << "  Dtool_PyInstDef *self = (Dtool_PyInstDef *)PyType_GenericAlloc(&Dtool_" << ClassName << "._PyType, 0);\n";
+      out << "  self->_signature = PY_PANDA_SIGNATURE;\n";
+      out << "  self->_My_Type = &Dtool_" << ClassName << ";\n";
+      out << "  self->_ptr_to_object = to_this;\n";
+      out << "  self->_memory_rules = false;\n";
+      out << "  self->_is_const = false;\n";
+      out << "  return (PyObject *)self;\n";
+    }
     out << "}\n\n";
   }
 }
@@ -1678,6 +1845,9 @@ write_module_class(ostream &out, Object *obj) {
   std::string cClassName =  obj->_itype.get_true_name();
   std::string export_class_name = classNameFromCppName(obj->_itype.get_name(), false);
 
+  CPPStructType *struct_type = obj->_itype._cpptype->as_struct_type();
+  bool py_subclassable = is_python_subclassable(struct_type);
+
   out << "/**\n";
   out << " * Python method tables for " << ClassName << " (" << export_class_name << ")\n" ;
   out << " */\n";
@@ -2558,22 +2728,36 @@ write_module_class(ostream &out, Object *obj) {
 
           // Find the remap.  There should be only one.
           FunctionRemap *remap = *def._remaps.begin();
-          const char *container = "";
 
-          if (remap->_has_this) {
-            out << "  " << cClassName << " *local_this = nullptr;\n";
-            out << "  DTOOL_Call_ExtractThisPointerForType(self, &Dtool_" << ClassName << ", (void **) &local_this);\n";
-            out << "  if (local_this == nullptr) {\n";
-            out << "    return 0;\n";
-            out << "  }\n\n";
-            container = "local_this";
+          out << "  " << cClassName << " *local_this = nullptr;\n";
+          out << "  DTOOL_Call_ExtractThisPointerForType(self, &Dtool_" << ClassName << ", (void **)&local_this);\n";
+          out << "  if (local_this == nullptr) {\n";
+          out << "    return 0;\n";
+          out << "  }\n\n";
+
+          // Only participate in cycle detection if there are no C++
+          // references to this object.
+          out << "  if (local_this->get_ref_count() != (int)((Dtool_PyInstDef *)self)->_memory_rules) {\n";
+          out << "    return 0;\n";
+          out << "  }\n\n";
+
+          if (py_subclassable) {
+            // Python-subclassable types store a reference to their Python
+            // wrapper.
+            if (struct_type->is_final()) {
+              out << "  if (Py_TYPE(self) != &Dtool_" << ClassName << "._PyType) {\n";
+            } else {
+              out << "  if (Py_TYPE(self) != &Dtool_" << ClassName << "._PyType && DtoolInstance_TYPE(self) == &Dtool_" << ClassName << ") {\n";
+            }
+            out << "    Py_VISIT(self);\n";
+            out << "  }\n";
           }
 
           vector_string params((int)remap->_has_this);
           params.push_back("visit");
           params.push_back("arg");
 
-          out << "  return " << remap->call_function(out, 2, false, container, params) << ";\n";
+          out << "  return " << remap->call_function(out, 2, false, "local_this", params) << ";\n";
           out << "}\n\n";
         }
         break;
@@ -2861,6 +3045,56 @@ write_module_class(ostream &out, Object *obj) {
     out << "}\n\n";
   }
 
+  bool has_gc = (slots.count("tp_traverse") || has_self_member(struct_type));
+  bool has_local_traverse = false;
+  bool has_local_clear = false;
+  if (py_subclassable || has_gc) {
+    // Note that in the case of py_subclassable, the traverse function will
+    // only be called if the class is subclassed from Python, since we don't
+    // set Py_TPFLAGS_HAVE_GC, so we don't need to check whether it's a proxy.
+    if (!slots.count("tp_traverse")) {
+      has_local_traverse = true;
+      out << "static int Dtool_Traverse_" << ClassName << "(PyObject *self, visitproc visit, void *arg) {\n";
+      out << "  // If the only reference remaining is the one held by the Python wrapper,\n";
+      out << "  // report the circular reference to Python's GC, so that it can break it.\n";
+      out << "  " << cClassName  << " *local_this = nullptr;\n";
+      out << "  if (!Dtool_Call_ExtractThisPointer(self, Dtool_" << ClassName << ", (void **)&local_this)) {\n";
+      out << "    return -1;\n";
+      out << "  }\n";
+      out << "  if (local_this->get_ref_count() == (int)((Dtool_PyInstDef *)self)->_memory_rules) {\n";
+      if (py_subclassable) {
+        out << "    Py_VISIT(self);\n";
+      }
+      if (has_self_member(struct_type)) {
+        out << "    Py_VISIT(local_this->__self__);\n";
+      }
+      out << "  }\n";
+      out << "  return 0;\n";
+      out << "}\n";
+      out << "\n";
+    }
+
+    if (!slots.count("tp_clear")) {
+      has_local_clear = true;
+      out << "static int Dtool_Clear_" << ClassName << "(PyObject *self) {\n";
+      if (has_self_member(struct_type)) {
+        out << "  Py_CLEAR(local_this->__self__);\n";
+      }
+      if (py_subclassable) {
+        if (struct_type->is_final()) {
+          out << "  if (Py_TYPE(self) != &Dtool_" << ClassName << "._PyType) {\n";
+        } else {
+          out << "  if (Py_TYPE(self) != &Dtool_" << ClassName << "._PyType && DtoolInstance_TYPE(self) == &Dtool_" << ClassName << ") {\n";
+        }
+        out << "    DtoolProxy_" << ClassName << " *proxy = (DtoolProxy_" << ClassName << " *)DtoolInstance_VOID_PTR(self);\n";
+        out << "    Py_CLEAR(proxy->DtoolProxy::_self);\n";
+        out << "  }\n";
+      }
+      out << "  return 0;\n";
+      out << "}\n\n";
+    }
+  }
+
   int num_getset = 0;
 
   if (obj->_properties.size() > 0) {
@@ -3151,7 +3385,7 @@ write_module_class(ostream &out, Object *obj) {
   }
 
   string gcflag;
-  if (obj->_protocol_types & Object::PT_python_gc) {
+  if (has_gc && slots.count("tp_traverse")) {
     gcflag = " | Py_TPFLAGS_HAVE_GC";
   }
 
@@ -3179,12 +3413,22 @@ write_module_class(ostream &out, Object *obj) {
   }
 
   // traverseproc tp_traverse;
-  out << "    nullptr, // tp_traverse\n";
-  //write_function_slot(out, 4, slots, "tp_traverse");
+  if (has_local_traverse) {
+    out << "    &Dtool_Traverse_" << ClassName << ",\n";
+  } else if (has_gc) {
+    write_function_slot(out, 4, slots, "tp_traverse");
+  } else {
+    out << "    nullptr, // tp_traverse\n";
+  }
 
   // inquiry tp_clear;
-  out << "    nullptr, // tp_clear\n";
-  //write_function_slot(out, 4, slots, "tp_clear");
+  if (has_local_clear) {
+    out << "    &Dtool_Clear_" << ClassName << ",\n";
+  } else if (has_gc) {
+    write_function_slot(out, 4, slots, "tp_clear");
+  } else {
+    out << "    nullptr, // tp_clear\n";
+  }
 
   // richcmpfunc tp_richcompare;
   if (has_local_richcompare) {
@@ -3237,7 +3481,7 @@ write_module_class(ostream &out, Object *obj) {
   // newfunc tp_new;
   write_function_slot(out, 4, slots, "tp_new", "Dtool_new_" + ClassName);
   // freefunc tp_free;
-  if (obj->_protocol_types & Object::PT_python_gc) {
+  if (slots.count("tp_traverse") || has_self_member(struct_type)) {
     out << "    PyObject_GC_Del,\n";
   } else {
     out << "    PyObject_Del,\n";
@@ -3277,9 +3521,9 @@ write_module_class(ostream &out, Object *obj) {
   out << "  Dtool_UpcastInterface_" << ClassName << ",\n";
   out << "  Dtool_Wrap_" << ClassName << ",\n";
 
-  int has_coerce = has_coerce_constructor(obj->_itype._cpptype->as_struct_type());
+  int has_coerce = has_coerce_constructor(struct_type);
   if (has_coerce > 0) {
-    if (TypeManager::is_reference_count(obj->_itype._cpptype)) {
+    if (TypeManager::is_reference_count(struct_type)) {
       out << "  (CoerceFunction)Dtool_ConstCoerce_" << ClassName << ",\n";
       if (has_coerce > 1) {
         out << "  (CoerceFunction)Dtool_Coerce_" << ClassName << ",\n";
@@ -6150,8 +6394,12 @@ write_function_instance(ostream &out, FunctionRemap *remap,
     indent_level += 2;
   }
 
-  if (is_constructor && !remap->_has_this &&
-      (remap->_flags & FunctionRemap::F_explicit_self) != 0) {
+  bool init_self_member = is_constructor &&
+    (return_flags & RF_coerced) == 0 &&
+    has_self_member(remap->_cpptype->as_struct_type());
+
+  if (init_self_member || (is_constructor && !remap->_has_this &&
+      (remap->_flags & FunctionRemap::F_explicit_self) != 0)) {
     // If we'll be passing "self" to the constructor, we need to pre-
     // initialize it here.  Unfortunately, we can't pre-load the "this"
     // pointer, but the constructor itself can do this.
@@ -6231,6 +6479,41 @@ write_function_instance(ostream &out, FunctionRemap *remap,
     }
     return_expr = "&coerced";
 
+  } else if (is_constructor && (return_flags & RF_coerced) == 0 &&
+             is_python_subclassable(remap->_cpptype->as_struct_type())) {
+    // Add special code to detect whether we're trying to create a subclass
+    // of this object.  If so, create a proxy instance instead.
+    CPPType *orig_type = remap->_return_type->get_orig_type();
+    TypeIndex type_index = builder.get_type(TypeManager::unwrap(TypeManager::resolve_type(orig_type)), false);
+    const InterrogateType &itype = idb->get_type(type_index);
+    std::string safe_name = make_safe_name(itype.get_scoped_name());
+
+    CPPType *type = remap->_return_type->get_temporary_type();
+    type->output_instance(indent(out, indent_level), "return_value", &parser);
+    out << ";\n";
+
+    indent(out, indent_level)
+      << "if (Py_TYPE(self) != &Dtool_" << safe_name << "._PyType) {\n";
+    indent(out, indent_level)
+      << "  DtoolProxy_" << safe_name << " *proxy = new DtoolProxy_" << safe_name << "(";
+    remap->write_call_args(out, pexprs);
+    out << ");\n";
+    indent(out, indent_level)
+      << "  DtoolProxy_Init(proxy, self, Dtool_" << safe_name << ", &Dtool_WrapProxy_" << safe_name << ");\n";
+    indent(out, indent_level)
+      << "  return_value = proxy;\n";
+    indent(out, indent_level)
+      << "} else {\n";
+
+    return_expr = remap->call_function(out, indent_level, true, container, pexprs);
+    indent(out, indent_level)
+      << "  return_value = " << return_expr << ";\n";
+    indent(out, indent_level)
+      << "}\n";
+
+    return_expr = "return_value";
+    manage_return = true;
+
   } else {
     // The general case; an ordinary constructor or function.
     return_expr = remap->call_function(out, indent_level, true, container, pexprs);
@@ -6290,6 +6573,11 @@ write_function_instance(ostream &out, FunctionRemap *remap,
       indent(out, indent_level) << "}\n";
     }
 
+    if (init_self_member) {
+      indent(out, indent_level)
+        << "return_value->__self__ = Py_NewRef(self);\n";
+    }
+
     if (TypeManager::is_pointer_to_PyObject(remap->_return_type->get_orig_type())) {
       return_expr = "Py_XNewRef(return_value)";
     } else {
@@ -8054,6 +8342,53 @@ IsRunTimeTyped(const InterrogateType &itype) {
   return false;
 }
 
+/**
+ * Returns true if this object has special support for inheriting from Python.
+ */
+bool InterfaceMakerPythonNative::
+is_python_subclassable(CPPStructType *struct_type) {
+  if (struct_type != nullptr && !struct_type->is_final() &&
+      IsPandaTypedObject(struct_type) &&
+      TypeManager::is_reference_count(struct_type)) {
+
+    // Make sure the type has a published constructor.
+    TypeIndex type_index = builder.get_type(struct_type, false);
+    if (type_index != 0) {
+      Objects::iterator oi = _objects.find(type_index);
+      if (oi != _objects.end()) {
+        Object *obj = (*oi).second;
+        return !obj->_constructors.empty();
+      }
+    }
+  }
+  return false;
+}
+
+/**
+ * Returns true if the type has a public member named __self__.
+ */
+bool InterfaceMakerPythonNative::
+has_self_member(CPPStructType *struct_type) {
+  if (struct_type == nullptr) {
+    return false;
+  }
+
+  CPPScope *scope = struct_type->get_scope();
+  if (scope != nullptr && scope->_variables.count("__self__")) {
+    return true;
+  }
+
+  for (const CPPStructType::Base &base : struct_type->_derivation) {
+    if (base._base != nullptr) {
+      CPPStructType *base_type = base._base->as_struct_type();
+      if (base_type != nullptr && has_self_member(base_type)) {
+        return true;
+      }
+    }
+  }
+  return false;
+}
+
 /**
 
  */

dtool/src/interrogate/interfaceMakerPythonNative.h

@@ -200,6 +200,8 @@ public:
   bool isExportThisRun(Function *func);
   bool isFunctionWithThis( Function *func);
   bool IsRunTimeTyped(const InterrogateType &itype);
+  bool is_python_subclassable(CPPStructType *type);
+  bool has_self_member(CPPStructType *type);
 
   // comunicates the cast capabilites among methods..
   struct CastDetails {

dtool/src/interrogatedb/py_compat.h

@@ -239,6 +239,10 @@ INLINE PyObject *PyObject_CallMethodNoArgs(PyObject *obj, PyObject *name) {
 INLINE PyObject *PyObject_CallMethodOneArg(PyObject *obj, PyObject *name, PyObject *arg) {
   return PyObject_CallMethodObjArgs(obj, name, arg, nullptr);
 }
+
+INLINE int PyObject_GC_IsTracked(PyObject *obj) {
+  return _PyObject_GC_IS_TRACKED(obj);
+}
 #endif
 
 /* Python 3.10 */

dtool/src/interrogatedb/py_panda.cxx

@@ -540,6 +540,32 @@ Dtool_TypeMap *Dtool_GetGlobalTypeMap() {
   }
 }
 
+/**
+ *
+ */
+void DtoolProxy_Init(DtoolProxy *proxy, PyObject *self,
+                     Dtool_PyTypedObject &classdef,
+                     TypeRegistry::PythonWrapFunc *wrap_func) {
+  if (proxy == nullptr) {
+    // Out of memory, the generated code will handle this.
+    return;
+  }
+
+  proxy->_self = Py_NewRef(self);
+  PyTypeObject *cls = Py_TYPE(self);
+  if (cls != &classdef._PyType) {
+    TypeRegistry *registry = TypeRegistry::ptr();
+    TypeHandle handle = registry->register_dynamic_type(cls->tp_name);
+    registry->record_derivation(handle, classdef._type);
+    //TODO unregister type when it is unloaded? weak callback?
+    PyTypeObject *cls_ref = (PyTypeObject *)Py_NewRef((PyObject *)cls);
+    registry->record_python_type(handle, cls_ref, wrap_func);
+    proxy->_type = handle;
+  } else {
+    proxy->_type = classdef._type;
+  }
+}
+
 #define PY_MAJOR_VERSION_STR #PY_MAJOR_VERSION "." #PY_MINOR_VERSION
 
 #if PY_MAJOR_VERSION >= 3

dtool/src/interrogatedb/py_panda.h

@@ -182,6 +182,16 @@ typedef std::map<std::string, Dtool_PyTypedObject *> Dtool_TypeMap;
 
 EXPCL_PYPANDA Dtool_TypeMap *Dtool_GetGlobalTypeMap();
 
+class DtoolProxy {
+public:
+  mutable PyObject *_self;
+  TypeHandle _type;
+};
+
+EXPCL_PYPANDA void DtoolProxy_Init(DtoolProxy *proxy, PyObject *self,
+                                   Dtool_PyTypedObject &classdef,
+                                   TypeRegistry::PythonWrapFunc *wrap_func);
+
 /**
 
  */

panda/src/putil/typedWritable_ext.cxx

@@ -31,7 +31,7 @@ extern Dtool_PyTypedObject Dtool_TypeHandle;
 /**
  * Class that upcalls to the parent class when write_datagram is called.
  */
-class TypedWritableProxy : public TypedWritable {
+class TypedWritableProxy : public TypedWritable, public DtoolProxy {
 public:
   ~TypedWritableProxy() {
   }
@@ -123,10 +123,6 @@ public:
   virtual TypeHandle force_init_type() override {
     return _type;
   }
-
-public:
-  PyObject *_self;
-  TypeHandle _type;
 };
 
 /**

panda/src/recorder/mouseRecorder.h

@@ -79,7 +79,7 @@ public:
 
   INLINE virtual int get_ref_count() const final { return ReferenceCount::get_ref_count(); };
   INLINE virtual void ref() const final { ReferenceCount::ref(); };
-  INLINE virtual bool unref() const final { return ReferenceCount::unref(); };
+  INLINE virtual bool unref() const { return ReferenceCount::unref(); };
 
 protected:
   static TypedWritable *make_from_bam(const FactoryParams &params);

tests/event/test_event.py

@@ -0,0 +1,64 @@
+from panda3d.core import Event, EventQueue
+from contextlib import contextmanager
+import gc
+
+
+@contextmanager
+def gc_disabled():
+    gc.disable()
+    gc.collect()
+    gc.freeze()
+    gc.set_debug(gc.DEBUG_SAVEALL)
+
+    try:
+        yield
+    finally:
+        gc.set_debug(0)
+        gc.garbage.clear()
+        gc.unfreeze()
+        gc.collect()
+        gc.enable()
+
+
+def test_event_subclass_gc():
+    class PythonEvent(Event):
+        pass
+
+    # Only subclasses should be tracked by the GC
+    assert not gc.is_tracked(Event('test'))
+
+    # Python wrapper destructs last
+    with gc_disabled():
+        event = PythonEvent('test_event_subclass_gc1')
+
+        assert len(gc.get_objects()) == 1
+        assert gc.get_objects()[0] == event
+
+        event = None
+
+        gc.collect()
+        assert len(gc.garbage) == 1
+        assert gc.garbage[0].name == 'test_event_subclass_gc1'
+
+    # C++ reference destructs last
+    with gc_disabled():
+        event = PythonEvent('test_event_subclass_gc2')
+        queue = EventQueue()
+        queue.queue_event(event)
+
+        assert event in gc.get_objects()
+
+        event = None
+
+        # Hasn't been collected yet, since parent still holds a reference
+        gc.collect()
+        assert len(gc.garbage) == 0
+        assert 'test_event_subclass_gc2' in [obj.name for obj in gc.get_objects() if isinstance(obj, Event)]
+
+        queue = None
+
+        # Destructed without needing the GC
+        assert 'test_event_subclass_gc2' not in [obj.name for obj in gc.get_objects() if isinstance(obj, Event)]
+
+        gc.collect()
+        assert len(gc.garbage) == 0

tests/pgraph/test_pandanode.py

@@ -1,4 +1,25 @@
 from panda3d.core import PandaNode, TransformState
+from contextlib import contextmanager
+import pytest
+import gc
+
+
+@contextmanager
+def gc_disabled():
+    gc.disable()
+    gc.collect()
+    gc.freeze()
+    gc.set_debug(gc.DEBUG_SAVEALL)
+    gc.garbage.clear()
+
+    try:
+        yield
+    finally:
+        gc.set_debug(0)
+        gc.garbage.clear()
+        gc.unfreeze()
+        gc.collect()
+        gc.enable()
 
 
 def test_node_prev_transform():
@@ -34,3 +55,123 @@ def test_node_prev_transform():
     assert node.transform == t3
     assert node.prev_transform == t3
     assert not node.has_dirty_prev_transform()
+
+
+def test_node_tag_cycle():
+    with gc_disabled():
+        node = PandaNode('test_node_tag_cycle')
+        node.set_python_tag('self', node)
+        node.set_python_tag('self2', node)
+        assert gc.is_tracked(node)
+        node = None
+
+        gc.collect()
+        assert len(gc.garbage) > 0
+
+        for g in gc.garbage:
+            if isinstance(g, PandaNode) and g.name == 'test_node_tag_cycle':
+                break
+        else:
+            assert False
+
+
[email protected]
+def test_node_tag_cycle_multiple_wrappers():
+    # Doesn't yet work since the traverse checks that the refcount is 1.
+    with gc_disabled():
+        node = PandaNode('test_node_tag_cycle_multiple_wrappers')
+        node.set_python_tag('self', node)
+
+        # Find another reference to the same node, we do this by temporarily
+        # attaching a child.
+        child = PandaNode('child')
+        node.add_child(child)
+        node2 = child.get_parent(0)
+        node.remove_child(0)
+        child = None
+
+        assert node2 == node
+        assert node2 is not node
+        assert node2.this == node.this
+
+        node.set_python_tag('self2', node2)
+
+        node = None
+        node2 = None
+
+        gc.collect()
+        assert len(gc.garbage) > 0
+
+        for g in gc.garbage:
+            if isinstance(g, PandaNode) and g.name == 'test_node_tag_cycle_multiple_wrappers':
+                break
+        else:
+            pytest.fail('not found in garbage')
+
+
+def test_node_subclass_persistent():
+    class NodeSubclass(PandaNode):
+        pass
+
+    node = NodeSubclass('test_node_subclass')
+    assert isinstance(node, PandaNode)
+
+    # Always GC-tracked
+    assert gc.is_tracked(node)
+
+    # Registered type handle
+    type_handle = node.get_type()
+    assert type_handle.name == 'NodeSubclass'
+    assert type_handle != PandaNode.get_class_type()
+    assert tuple(type_handle.parent_classes) == (PandaNode.get_class_type(), )
+
+    # Persistent wrapper
+    parent = PandaNode('parent')
+    parent.add_child(node)
+    child = parent.get_child(0)
+    assert child.this == node.this
+    assert child is node
+    assert type(child) is NodeSubclass
+    parent = None
+    child = None
+
+
+def test_node_subclass_gc():
+    class NodeSubclass(PandaNode):
+        pass
+
+    # Python wrapper destructs last
+    with gc_disabled():
+        node = NodeSubclass('test_node_subclass_gc1')
+
+        assert len(gc.get_objects()) == 1
+        assert gc.get_objects()[0] == node
+
+        node = None
+
+        gc.collect()
+        assert len(gc.garbage) == 1
+        assert gc.garbage[0].name == 'test_node_subclass_gc1'
+
+    # C++ reference destructs last
+    with gc_disabled():
+        node = NodeSubclass('test_node_subclass_gc2')
+        parent = PandaNode('parent')
+        parent.add_child(node)
+
+        assert node in gc.get_objects()
+
+        node = None
+
+        # Hasn't been collected yet, since parent still holds a reference
+        gc.collect()
+        assert len(gc.garbage) == 0
+        assert 'test_node_subclass_gc2' in [obj.name for obj in gc.get_objects() if isinstance(obj, PandaNode)]
+
+        parent = None
+
+        # Destructed without needing the GC
+        assert 'test_node_subclass_gc2' not in [obj.name for obj in gc.get_objects() if isinstance(obj, PandaNode)]
+
+        gc.collect()
+        assert len(gc.garbage) == 0