fix leak due to cyclic RenderState reference counts

panda/src/pgraph/colorScaleAttrib.cxx

@@ -118,9 +118,21 @@ compare_to_impl(const RenderAttrib *other) const {
   DCAST_INTO_R(ta, other, 0);
 
   if (is_off() != ta->is_off()) {
+    if (pgraph_cat.is_spam()) {
+      pgraph_cat.spam()
+        << "Comparing " << (int)is_off() << " to " << (int)ta->is_off() << " result = "
+        << (int)is_off() - (int)ta->is_off() << "\n";
+    }
+    
     return (int)is_off() - (int)ta->is_off();
   }
 
+  if (pgraph_cat.is_spam()) {
+    pgraph_cat.spam()
+      << "Comparing " << _scale << " to " << ta->_scale << " result = "
+      << _scale.compare_to(ta->_scale) << "\n";
+  }
+
   return _scale.compare_to(ta->_scale);
 }
 

panda/src/pgraph/config_pgraph.cxx

@@ -127,6 +127,12 @@ const bool paranoid_compose = config_pgraph.GetBool("paranoid-compose", false);
 // by matrix.
 const bool compose_componentwise = config_pgraph.GetBool("compose-componentwise", true);
 
+// Set this true to double-check that nothing is inappropriately
+// modifying the supposedly const structures like RenderState,
+// RenderAttrib, TransformState, and RenderEffect.  This has no effect
+// if NDEBUG is defined.
+const bool paranoid_const = config_pgraph.GetBool("paranoid-const", false);
+
 // Set this true to view some info statements regarding the polylight
 // It is helpful for debugging
 const bool polylight_info = config_pgraph.GetBool("polylight-info", false);

panda/src/pgraph/config_pgraph.h

@@ -35,6 +35,7 @@ extern const bool unambiguous_graph;
 extern const bool allow_unrelated_wrt;
 extern const bool paranoid_compose;
 extern const bool compose_componentwise;
+extern const bool paranoid_const;
 
 extern const bool polylight_info;
 

panda/src/pgraph/renderAttrib.I

@@ -68,8 +68,22 @@ make_default() const {
 }
 
 ////////////////////////////////////////////////////////////////////
-//     Function: RenderAttrib::compare_to
+//     Function: RenderAttrib::always_reissue
 //       Access: Public
+//  Description: Returns true if the RenderAttrib should be reissued
+//               to the GSG with every state change, even if it is the
+//               same pointer as it was before; or false for the
+//               normal case, to reissue only when the RenderAttrib
+//               pointer changes.
+////////////////////////////////////////////////////////////////////
+INLINE bool RenderAttrib::
+always_reissue() const {
+  return _always_reissue;
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: RenderAttrib::compare_to
+//       Access: Published
 //  Description: Provides an arbitrary ordering among all unique
 //               RenderAttribs, so we can store the essentially
 //               different ones in a big set and throw away the rest.
@@ -92,18 +106,3 @@ compare_to(const RenderAttrib &other) const {
   // We only call compare_to_impl() if they have the same type.
   return compare_to_impl(&other);
 }
-
-////////////////////////////////////////////////////////////////////
-//     Function: RenderAttrib::always_reissue
-//       Access: Public
-//  Description: Returns true if the RenderAttrib should be reissued
-//               to the GSG with every state change, even if it is the
-//               same pointer as it was before; or false for the
-//               normal case, to reissue only when the RenderAttrib
-//               pointer changes.
-////////////////////////////////////////////////////////////////////
-INLINE bool RenderAttrib::
-always_reissue() const {
-  return _always_reissue;
-}
-

panda/src/pgraph/renderAttrib.cxx

@@ -124,6 +124,70 @@ write(ostream &out, int indent_level) const {
   indent(out, indent_level) << *this << "\n";
 }
 
+////////////////////////////////////////////////////////////////////
+//     Function: RenderAttrib::get_num_attribs
+//       Access: Published, Static
+//  Description: Returns the total number of unique RenderAttrib
+//               objects allocated in the world.  This will go up and
+//               down during normal operations.
+////////////////////////////////////////////////////////////////////
+int RenderAttrib::
+get_num_attribs() {
+  if (_attribs == (Attribs *)NULL) {
+    return 0;
+  }
+  return _attribs->size();
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: RenderAttrib::list_attribs
+//       Access: Published, Static
+//  Description: Lists all of the RenderAttribs in the cache to the
+//               output stream, one per line.  This can be quite a lot
+//               of output if the cache is large, so be prepared.
+////////////////////////////////////////////////////////////////////
+void RenderAttrib::
+list_attribs(ostream &out) {
+  out << _attribs->size() << " attribs:\n";
+  Attribs::const_iterator si;
+  for (si = _attribs->begin(); si != _attribs->end(); ++si) {
+    const RenderAttrib *attrib = (*si);
+    attrib->write(out, 2);
+  }
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: RenderAttrib::validate_attribs
+//       Access: Published, Static
+//  Description: Ensures that the cache is still stored in sorted
+//               order.  Returns true if so, false if there is a
+//               problem (which implies someone has modified one of
+//               the supposedly-const RenderAttrib objects).
+////////////////////////////////////////////////////////////////////
+bool RenderAttrib::
+validate_attribs() {
+  if (_attribs->empty()) {
+    return true;
+  }
+
+  Attribs::const_iterator si = _attribs->begin();
+  Attribs::const_iterator snext = si;
+  ++snext;
+  while (snext != _attribs->end()) {
+    if ((*si)->compare_to(*(*snext)) >= 0) {
+      pgraph_cat.error()
+        << "RenderAttribs out of order!\n";
+      (*si)->write(pgraph_cat.error(false), 2);
+      (*snext)->write(pgraph_cat.error(false), 2);
+      return false;
+    }
+    si = snext;
+    ++snext;
+  }
+
+  return true;
+}
+
 ////////////////////////////////////////////////////////////////////
 //     Function: RenderAttrib::return_new
 //       Access: Protected, Static
@@ -146,6 +210,12 @@ return_new(RenderAttrib *attrib) {
   // for anything else.
   nassertr(attrib->_saved_entry == _attribs->end(), attrib);
 
+#ifndef NDEBUG
+  if (paranoid_const) {
+    nassertr(validate_attribs(), attrib);
+  }
+#endif
+
   // Save the attrib in a local PointerTo so that it will be freed at
   // the end of this function if no one else uses it.
   CPT(RenderAttrib) pt_attrib = attrib;

panda/src/pgraph/renderAttrib.h

@@ -68,15 +68,19 @@ public:
   INLINE CPT(RenderAttrib) compose(const RenderAttrib *other) const;
   INLINE CPT(RenderAttrib) invert_compose(const RenderAttrib *other) const;
   INLINE CPT(RenderAttrib) make_default() const;
-  INLINE int compare_to(const RenderAttrib &other) const;
   virtual void issue(GraphicsStateGuardianBase *gsg) const;
 
   INLINE bool always_reissue() const;
 
 PUBLISHED:
+  INLINE int compare_to(const RenderAttrib &other) const;
   virtual void output(ostream &out) const;
   virtual void write(ostream &out, int indent_level) const;
 
+  static int get_num_attribs();
+  static void list_attribs(ostream &out);
+  static bool validate_attribs();
+
   enum PandaCompareFunc {   // intentionally defined to match D3DCMPFUNC
     M_none=0,           // alpha-test disabled (always-draw)
     M_never,            // Never draw.

panda/src/pgraph/renderEffect.I

@@ -19,7 +19,7 @@
 
 ////////////////////////////////////////////////////////////////////
 //     Function: RenderEffect::compare_to
-//       Access: Public
+//       Access: Published
 //  Description: Provides an arbitrary ordering among all unique
 //               RenderEffects, so we can store the essentially
 //               different ones in a big set and throw away the rest.

panda/src/pgraph/renderEffect.cxx

@@ -20,7 +20,7 @@
 #include "bamReader.h"
 #include "indent.h"
 
-RenderEffect::Effects RenderEffect::_effects;
+RenderEffect::Effects *RenderEffect::_effects = NULL;
 TypeHandle RenderEffect::_type_handle;
 
 ////////////////////////////////////////////////////////////////////
@@ -30,7 +30,15 @@ TypeHandle RenderEffect::_type_handle;
 ////////////////////////////////////////////////////////////////////
 RenderEffect::
 RenderEffect() {
-  _saved_entry = _effects.end();
+  if (_effects == (Effects *)NULL) {
+    // Make sure the global _effects map is allocated.  This only has
+    // to be done once.  We could make this map static, but then we
+    // run into problems if anyone creates a RenderState object at
+    // static init time; it also seems to cause problems when the
+    // Panda shared library is unloaded at application exit time.
+    _effects = new Effects;
+  }
+  _saved_entry = _effects->end();
 }
 
 ////////////////////////////////////////////////////////////////////
@@ -61,13 +69,13 @@ operator = (const RenderEffect &) {
 ////////////////////////////////////////////////////////////////////
 RenderEffect::
 ~RenderEffect() {
-  if (_saved_entry != _effects.end()) {
+  if (_saved_entry != _effects->end()) {
     // We cannot make this assertion, because the RenderEffect has
     // already partially destructed--this means we cannot look up the
     // object in the map.  In fact, the map is temporarily invalid
     // until we finish destructing, since we screwed up the ordering
     // when we changed the return value of get_type().
-    //    nassertv(_effects.find(this) == _saved_entry);
+    //    nassertv(_effects->find(this) == _saved_entry);
 
     // Note: this isn't thread-safe, because once the derived class
     // destructor exits and before this destructor completes, the map
@@ -76,8 +84,8 @@ RenderEffect::
     // functionality to a separate method, that is to be called from
     // *each* derived class's destructor (and then we can put the
     // above assert back in).
-    _effects.erase(_saved_entry);
-    _saved_entry = _effects.end();
+    _effects->erase(_saved_entry);
+    _saved_entry = _effects->end();
   }
 }
 
@@ -172,6 +180,70 @@ write(ostream &out, int indent_level) const {
   indent(out, indent_level) << *this << "\n";
 }
 
+////////////////////////////////////////////////////////////////////
+//     Function: RenderEffect::get_num_effects
+//       Access: Published, Static
+//  Description: Returns the total number of unique RenderEffect
+//               objects allocated in the world.  This will go up and
+//               down during normal operations.
+////////////////////////////////////////////////////////////////////
+int RenderEffect::
+get_num_effects() {
+  if (_effects == (Effects *)NULL) {
+    return 0;
+  }
+  return _effects->size();
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: RenderEffect::list_effects
+//       Access: Published, Static
+//  Description: Lists all of the RenderEffects in the cache to the
+//               output stream, one per line.  This can be quite a lot
+//               of output if the cache is large, so be prepared.
+////////////////////////////////////////////////////////////////////
+void RenderEffect::
+list_effects(ostream &out) {
+  out << _effects->size() << " effects:\n";
+  Effects::const_iterator si;
+  for (si = _effects->begin(); si != _effects->end(); ++si) {
+    const RenderEffect *effect = (*si);
+    effect->write(out, 2);
+  }
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: RenderEffect::validate_effects
+//       Access: Published, Static
+//  Description: Ensures that the cache is still stored in sorted
+//               order.  Returns true if so, false if there is a
+//               problem (which implies someone has modified one of
+//               the supposedly-const RenderEffect objects).
+////////////////////////////////////////////////////////////////////
+bool RenderEffect::
+validate_effects() {
+  if (_effects->empty()) {
+    return true;
+  }
+
+  Effects::const_iterator si = _effects->begin();
+  Effects::const_iterator snext = si;
+  ++snext;
+  while (snext != _effects->end()) {
+    if ((*si)->compare_to(*(*snext)) >= 0) {
+      pgraph_cat.error()
+        << "RenderEffects out of order!\n";
+      (*si)->write(pgraph_cat.error(false), 2);
+      (*snext)->write(pgraph_cat.error(false), 2);
+      return false;
+    }
+    si = snext;
+    ++snext;
+  }
+
+  return true;
+}
+
 ////////////////////////////////////////////////////////////////////
 //     Function: RenderEffect::return_new
 //       Access: Protected, Static
@@ -192,13 +264,19 @@ return_new(RenderEffect *effect) {
 
   // This should be a newly allocated pointer, not one that was used
   // for anything else.
-  nassertr(effect->_saved_entry == _effects.end(), effect);
+  nassertr(effect->_saved_entry == _effects->end(), effect);
+
+#ifndef NDEBUG
+  if (paranoid_const) {
+    nassertr(validate_effects(), effect);
+  }
+#endif
 
   // Save the effect in a local PointerTo so that it will be freed at
   // the end of this function if no one else uses it.
   CPT(RenderEffect) pt_effect = effect;
 
-  pair<Effects::iterator, bool> result = _effects.insert(effect);
+  pair<Effects::iterator, bool> result = _effects->insert(effect);
   if (result.second) {
     // The effect was inserted; save the iterator and return the
     // input effect.

panda/src/pgraph/renderEffect.h

@@ -67,8 +67,6 @@ private:
 public:
   virtual ~RenderEffect();
 
-  INLINE int compare_to(const RenderEffect &other) const;
-
   virtual bool safe_to_transform() const;
   virtual bool safe_to_combine() const;
   virtual CPT(RenderEffect) xform(const LMatrix4f &mat) const;
@@ -79,9 +77,15 @@ public:
                              CPT(RenderState) &node_state) const;
 
 PUBLISHED:
+  INLINE int compare_to(const RenderEffect &other) const;
+
   virtual void output(ostream &out) const;
   virtual void write(ostream &out, int indent_level) const;
 
+  static int get_num_effects();
+  static void list_effects(ostream &out);
+  static bool validate_effects();
+
 protected:
   static CPT(RenderEffect) return_new(RenderEffect *effect);
 
@@ -89,7 +93,7 @@ protected:
 
 private:
   typedef pset<const RenderEffect *, IndirectCompareTo<RenderEffect> > Effects;
-  static Effects _effects;
+  static Effects *_effects;
 
   Effects::iterator _saved_entry;
 

panda/src/pgraph/renderEffects.cxx

@@ -29,7 +29,7 @@
 #include "indent.h"
 #include "compareTo.h"
 
-RenderEffects::States RenderEffects::_states;
+RenderEffects::States *RenderEffects::_states = NULL;
 CPT(RenderEffects) RenderEffects::_empty_state;
 TypeHandle RenderEffects::_type_handle;
 
@@ -42,7 +42,15 @@ TypeHandle RenderEffects::_type_handle;
 ////////////////////////////////////////////////////////////////////
 RenderEffects::
 RenderEffects() {
-  _saved_entry = _states.end();
+  if (_states == (States *)NULL) {
+    // Make sure the global _states map is allocated.  This only has
+    // to be done once.  We could make this map static, but then we
+    // run into problems if anyone creates a RenderState object at
+    // static init time; it also seems to cause problems when the
+    // Panda shared library is unloaded at application exit time.
+    _states = new States;
+  }
+  _saved_entry = _states->end();
   _flags = 0;
 }
 
@@ -75,33 +83,12 @@ operator = (const RenderEffects &) {
 RenderEffects::
 ~RenderEffects() {
   // Remove the deleted RenderEffects object from the global pool.
-  if (_saved_entry != _states.end()) {
-    _states.erase(_saved_entry);
-    _saved_entry = _states.end();
+  if (_saved_entry != _states->end()) {
+    _states->erase(_saved_entry);
+    _saved_entry = _states->end();
   }
 }
 
-////////////////////////////////////////////////////////////////////
-//     Function: RenderEffects::operator <
-//       Access: Public
-//  Description: Provides an arbitrary ordering among all unique
-//               RenderEffectss, so we can store the essentially
-//               different ones in a big set and throw away the rest.
-//
-//               This method is not needed outside of the RenderEffects
-//               class because all equivalent RenderEffects objects are
-//               guaranteed to share the same pointer; thus, a pointer
-//               comparison is always sufficient.
-////////////////////////////////////////////////////////////////////
-bool RenderEffects::
-operator < (const RenderEffects &other) const {
-  // We must compare all the properties of the effects, not just
-  // the type; thus, we compare them one at a time using compare_to().
-  return lexicographical_compare(_effects.begin(), _effects.end(),
-                                 other._effects.begin(), other._effects.end(),
-                                 CompareTo<Effect>());
-}
-
 ////////////////////////////////////////////////////////////////////
 //     Function: RenderEffects::safe_to_transform
 //       Access: Public
@@ -171,6 +158,27 @@ xform(const LMatrix4f &mat) const {
   return return_new(new_state);
 }
 
+////////////////////////////////////////////////////////////////////
+//     Function: RenderEffects::operator <
+//       Access: Published
+//  Description: Provides an arbitrary ordering among all unique
+//               RenderEffectss, so we can store the essentially
+//               different ones in a big set and throw away the rest.
+//
+//               This method is not needed outside of the RenderEffects
+//               class because all equivalent RenderEffects objects are
+//               guaranteed to share the same pointer; thus, a pointer
+//               comparison is always sufficient.
+////////////////////////////////////////////////////////////////////
+bool RenderEffects::
+operator < (const RenderEffects &other) const {
+  // We must compare all the properties of the effects, not just
+  // the type; thus, we compare them one at a time using compare_to().
+  return lexicographical_compare(_effects.begin(), _effects.end(),
+                                 other._effects.begin(), other._effects.end(),
+                                 CompareTo<Effect>());
+}
+
 
 ////////////////////////////////////////////////////////////////////
 //     Function: RenderEffects::find_effect
@@ -412,6 +420,70 @@ cull_callback(CullTraverser *trav, CullTraverserData &data,
   }
 }
 
+////////////////////////////////////////////////////////////////////
+//     Function: RenderEffects::get_num_states
+//       Access: Published, Static
+//  Description: Returns the total number of unique RenderEffects
+//               objects allocated in the world.  This will go up and
+//               down during normal operations.
+////////////////////////////////////////////////////////////////////
+int RenderEffects::
+get_num_states() {
+  if (_states == (States *)NULL) {
+    return 0;
+  }
+  return _states->size();
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: RenderEffects::list_states
+//       Access: Published, Static
+//  Description: Lists all of the RenderEffectss in the cache to the
+//               output stream, one per line.  This can be quite a lot
+//               of output if the cache is large, so be prepared.
+////////////////////////////////////////////////////////////////////
+void RenderEffects::
+list_states(ostream &out) {
+  out << _states->size() << " states:\n";
+  States::const_iterator si;
+  for (si = _states->begin(); si != _states->end(); ++si) {
+    const RenderEffects *state = (*si);
+    state->write(out, 2);
+  }
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: RenderEffects::validate_states
+//       Access: Published, Static
+//  Description: Ensures that the cache is still stored in sorted
+//               order.  Returns true if so, false if there is a
+//               problem (which implies someone has modified one of
+//               the supposedly-const RenderEffects objects).
+////////////////////////////////////////////////////////////////////
+bool RenderEffects::
+validate_states() {
+  if (_states->empty()) {
+    return true;
+  }
+
+  States::const_iterator si = _states->begin();
+  States::const_iterator snext = si;
+  ++snext;
+  while (snext != _states->end()) {
+    if (!(*(*si) < *(*snext))) {
+      pgraph_cat.error()
+        << "RenderEffectss out of order!\n";
+      (*si)->write(pgraph_cat.error(false), 2);
+      (*snext)->write(pgraph_cat.error(false), 2);
+      return false;
+    }
+    si = snext;
+    ++snext;
+  }
+
+  return true;
+}
+
 ////////////////////////////////////////////////////////////////////
 //     Function: RenderEffects::return_new
 //       Access: Private, Static
@@ -430,13 +502,19 @@ return_new(RenderEffects *state) {
 
   // This should be a newly allocated pointer, not one that was used
   // for anything else.
-  nassertr(state->_saved_entry == _states.end(), state);
+  nassertr(state->_saved_entry == _states->end(), state);
+
+#ifndef NDEBUG
+  if (paranoid_const) {
+    nassertr(validate_states(), state);
+  }
+#endif
 
   // Save the state in a local PointerTo so that it will be freed at
   // the end of this function if no one else uses it.
   CPT(RenderEffects) pt_state = state;
 
-  pair<States::iterator, bool> result = _states.insert(state);
+  pair<States::iterator, bool> result = _states->insert(state);
   if (result.second) {
     // The state was inserted; save the iterator and return the
     // input state.

panda/src/pgraph/renderEffects.h

@@ -57,13 +57,13 @@ private:
 public:
   virtual ~RenderEffects();
 
-  bool operator < (const RenderEffects &other) const;
-
   bool safe_to_transform() const;
   bool safe_to_combine() const;
   CPT(RenderEffects) xform(const LMatrix4f &mat) const;
 
 PUBLISHED:
+  bool operator < (const RenderEffects &other) const;
+
   INLINE bool is_empty() const;
   INLINE int get_num_effects() const;
   INLINE const RenderEffect *get_effect(int n) const;
@@ -90,6 +90,10 @@ PUBLISHED:
   void output(ostream &out) const;
   void write(ostream &out, int indent_level) const;
 
+  static int get_num_states();
+  static void list_states(ostream &out);
+  static bool validate_states();
+
 public:
   INLINE bool has_decal() const;
   INLINE bool has_show_bounds() const;
@@ -107,7 +111,7 @@ private:
 
 private:
   typedef pset<const RenderEffects *, IndirectLess<RenderEffects> > States;
-  static States _states;
+  static States *_states;
   static CPT(RenderEffects) _empty_state;
 
   // This iterator records the entry corresponding to this RenderEffects

panda/src/pgraph/renderState.I

@@ -17,127 +17,6 @@
 ////////////////////////////////////////////////////////////////////
 
 
-////////////////////////////////////////////////////////////////////
-//     Function: RenderState::Composition::Constructor
-//       Access: Public
-//  Description: 
-////////////////////////////////////////////////////////////////////
-INLINE RenderState::Composition::
-Composition() {
-}
-
-////////////////////////////////////////////////////////////////////
-//     Function: RenderState::Composition::Copy Constructor
-//       Access: Public
-//  Description: 
-////////////////////////////////////////////////////////////////////
-INLINE RenderState::Composition::
-Composition(const RenderState::Composition &copy) :
-  _result(copy._result)
-{
-}
-
-////////////////////////////////////////////////////////////////////
-//     Function: RenderState::Attribute::Constructor
-//       Access: Public
-//  Description:
-////////////////////////////////////////////////////////////////////
-INLINE RenderState::Attribute::
-Attribute(const RenderAttrib *attrib, int override) :
-  _type(attrib->get_type()),
-  _attrib(attrib),
-  _override(override)
-{
-}
-
-////////////////////////////////////////////////////////////////////
-//     Function: RenderState::Attribute::Constructor
-//       Access: Public
-//  Description: This constructor is only used when reading the
-//               RenderState from a bam file.  At this point, the
-//               attribute pointer is unknown.
-////////////////////////////////////////////////////////////////////
-INLINE RenderState::Attribute::
-Attribute(int override) :
-  _override(override)
-{
-}
-
-////////////////////////////////////////////////////////////////////
-//     Function: RenderState::Attribute::Constructor
-//       Access: Public
-//  Description: This constructor makes an invalid Attribute with no
-//               RenderAttrib pointer; its purpose is just to make an
-//               object we can use to look up a particular type in the
-//               Attribute set.
-////////////////////////////////////////////////////////////////////
-INLINE RenderState::Attribute::
-Attribute(TypeHandle type) :
-  _type(type),
-  _attrib(NULL),
-  _override(0)
-{
-}
-
-////////////////////////////////////////////////////////////////////
-//     Function: RenderState::Attribute::Copy Constructor
-//       Access: Public
-//  Description:
-////////////////////////////////////////////////////////////////////
-INLINE RenderState::Attribute::
-Attribute(const Attribute &copy) :
-  _type(copy._type),
-  _attrib(copy._attrib),
-  _override(copy._override)
-{
-}
-
-////////////////////////////////////////////////////////////////////
-//     Function: RenderState::Attribute::Copy Assignment Operator
-//       Access: Public
-//  Description:
-////////////////////////////////////////////////////////////////////
-INLINE void RenderState::Attribute::
-operator = (const Attribute &copy) {
-  _type = copy._type;
-  _attrib = copy._attrib;
-  _override = copy._override;
-}
-
-////////////////////////////////////////////////////////////////////
-//     Function: RenderState::Attribute::operator <
-//       Access: Public
-//  Description: This is used by the Attributes set to uniquify
-//               RenderAttributes by type.  Only one RenderAttrib of a
-//               given type is allowed in the set.  This ordering must
-//               also match the ordering reported by compare_to().
-////////////////////////////////////////////////////////////////////
-INLINE bool RenderState::Attribute::
-operator < (const Attribute &other) const {
-  return _type < other._type;
-}
-
-////////////////////////////////////////////////////////////////////
-//     Function: RenderState::Attribute::compare_to
-//       Access: Public
-//  Description: Provides an indication of whether a particular
-//               attribute is equivalent to another one, for purposes
-//               of generating unique RenderStates.  This should
-//               compare all properties of the Attribute, but it is
-//               important that the type is compared first, to be
-//               consistent with the ordering defined by operator <.
-////////////////////////////////////////////////////////////////////
-INLINE int RenderState::Attribute::
-compare_to(const Attribute &other) const {
-  if (_type != other._type) {
-    return _type.get_index() - other._type.get_index();
-  }
-  if (_attrib != other._attrib) {
-    return _attrib < other._attrib ? -1 : 1;
-  }
-  return _override - other._override;
-}
-
 ////////////////////////////////////////////////////////////////////
 //     Function: RenderState::is_empty
 //       Access: Published
@@ -308,3 +187,165 @@ is_destructing() const {
   return false;
 #endif
 }
+
+////////////////////////////////////////////////////////////////////
+//     Function: RenderState::Composition::Constructor
+//       Access: Public
+//  Description: 
+////////////////////////////////////////////////////////////////////
+INLINE RenderState::Composition::
+Composition() {
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: RenderState::Composition::Copy Constructor
+//       Access: Public
+//  Description: 
+////////////////////////////////////////////////////////////////////
+INLINE RenderState::Composition::
+Composition(const RenderState::Composition &copy) :
+  _result(copy._result)
+{
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: RenderState::Attribute::Constructor
+//       Access: Public
+//  Description:
+////////////////////////////////////////////////////////////////////
+INLINE RenderState::Attribute::
+Attribute(const RenderAttrib *attrib, int override) :
+  _type(attrib->get_type()),
+  _attrib(attrib),
+  _override(override)
+{
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: RenderState::Attribute::Constructor
+//       Access: Public
+//  Description: This constructor is only used when reading the
+//               RenderState from a bam file.  At this point, the
+//               attribute pointer is unknown.
+////////////////////////////////////////////////////////////////////
+INLINE RenderState::Attribute::
+Attribute(int override) :
+  _override(override)
+{
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: RenderState::Attribute::Constructor
+//       Access: Public
+//  Description: This constructor makes an invalid Attribute with no
+//               RenderAttrib pointer; its purpose is just to make an
+//               object we can use to look up a particular type in the
+//               Attribute set.
+////////////////////////////////////////////////////////////////////
+INLINE RenderState::Attribute::
+Attribute(TypeHandle type) :
+  _type(type),
+  _attrib(NULL),
+  _override(0)
+{
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: RenderState::Attribute::Copy Constructor
+//       Access: Public
+//  Description:
+////////////////////////////////////////////////////////////////////
+INLINE RenderState::Attribute::
+Attribute(const Attribute &copy) :
+  _type(copy._type),
+  _attrib(copy._attrib),
+  _override(copy._override)
+{
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: RenderState::Attribute::Copy Assignment Operator
+//       Access: Public
+//  Description:
+////////////////////////////////////////////////////////////////////
+INLINE void RenderState::Attribute::
+operator = (const Attribute &copy) {
+  _type = copy._type;
+  _attrib = copy._attrib;
+  _override = copy._override;
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: RenderState::Attribute::operator <
+//       Access: Public
+//  Description: This is used by the Attributes set to uniquify
+//               RenderAttributes by type.  Only one RenderAttrib of a
+//               given type is allowed in the set.  This ordering must
+//               also match the ordering reported by compare_to().
+////////////////////////////////////////////////////////////////////
+INLINE bool RenderState::Attribute::
+operator < (const Attribute &other) const {
+  return _type < other._type;
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: RenderState::Attribute::compare_to
+//       Access: Public
+//  Description: Provides an indication of whether a particular
+//               attribute is equivalent to another one, for purposes
+//               of generating unique RenderStates.  This should
+//               compare all properties of the Attribute, but it is
+//               important that the type is compared first, to be
+//               consistent with the ordering defined by operator <.
+////////////////////////////////////////////////////////////////////
+INLINE int RenderState::Attribute::
+compare_to(const Attribute &other) const {
+  if (_type != other._type) {
+    return _type.get_index() - other._type.get_index();
+  }
+  if (_attrib != other._attrib) {
+    return _attrib < other._attrib ? -1 : 1;
+  }
+  return _override - other._override;
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: RenderState::CompositionCycleDescEntry::Constructor
+//       Access: Public
+//  Description: 
+////////////////////////////////////////////////////////////////////
+INLINE RenderState::CompositionCycleDescEntry::
+CompositionCycleDescEntry(const RenderState *obj,
+                          const RenderState *result,
+                          bool inverted) :
+  _obj(obj),
+  _result(result),
+  _inverted(inverted)
+{
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: RenderState::CycleChain::Constructor
+//       Access: Public
+//  Description: 
+////////////////////////////////////////////////////////////////////
+INLINE RenderState::CycleChain::
+CycleChain(const RenderState *state) :
+  _state(state),
+  _prev(NULL),
+  _length(1)
+{
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: RenderState::CycleChain::Constructor
+//       Access: Public
+//  Description: 
+////////////////////////////////////////////////////////////////////
+INLINE RenderState::CycleChain::
+CycleChain(CycleChain *prev, const RenderState *state) :
+  _state(state),
+  _prev(prev),
+  _length(prev->_length + 1)
+{
+}

panda/src/pgraph/renderState.cxx

@@ -52,7 +52,6 @@ RenderState() {
     _states = new States;
   }
   _saved_entry = _states->end();
-  _self_compose = (RenderState *)NULL;
   _flags = 0;
 }
 
@@ -127,13 +126,8 @@ RenderState::
     // objects destruct, there will be no reason for that one to.
     RenderState *other = (RenderState *)(*ci).first;
 
-    // We should never have a reflexive entry in this map.  If we
-    // do, something got screwed up elsewhere.
-    nassertv(other != this);
-
-    // We hold a copy of the composition result to ensure that the
-    // result RenderState object (if there is one) doesn't
-    // destruct.
+    // We hold a copy of the composition result so we can dereference
+    // it later.
     Composition comp = (*ci).second;
 
     // Now we can remove the element from our cache.  We do this now,
@@ -142,25 +136,33 @@ RenderState::
     // is still valid.
     _composition_cache.erase(ci);
 
-    CompositionCache::iterator oci = other->_composition_cache.find(this);
-
-    // We may or may not still be listed in the other's cache (it
-    // might be halfway through pulling entries out, from within its
-    // own destructor).
-    if (oci != other->_composition_cache.end()) {
-      // Hold a copy of the other composition result, too.
-      Composition ocomp = (*oci).second;
-      
-      // Now we're holding a reference count to both computed
-      // results, so no objects will be tempted to destruct while we
-      // erase the other cache entry.
-      other->_composition_cache.erase(oci);
+    if (other != this) {
+      CompositionCache::iterator oci = other->_composition_cache.find(this);
+
+      // We may or may not still be listed in the other's cache (it
+      // might be halfway through pulling entries out, from within its
+      // own destructor).
+      if (oci != other->_composition_cache.end()) {
+        // Hold a copy of the other composition result, too.
+        Composition ocomp = (*oci).second;
+        
+        other->_composition_cache.erase(oci);
+        
+        // It's finally safe to let our held pointers go away.  This may
+        // have cascading effects as other RenderState objects are
+        // destructed, but there will be no harm done if they destruct
+        // now.
+        if (ocomp._result != (const RenderState *)NULL && ocomp._result != other) {
+          unref_delete(ocomp._result);
+        }
+      }
     }
 
-    // It's finally safe to let our held pointers go away.  This may
-    // have cascading effects as other RenderState objects are
-    // destructed, but there will be no harm done if they destruct
-    // now.
+    // It's finally safe to let our held pointers go away.  (See
+    // comment above.)
+    if (comp._result != (const RenderState *)NULL && comp._result != this) {
+      unref_delete(comp._result);
+    }
   }
 
   // A similar bit of code for the invert cache.
@@ -170,25 +172,30 @@ RenderState::
     nassertv(other != this);
     Composition comp = (*ci).second;
     _invert_composition_cache.erase(ci);
-    CompositionCache::iterator oci = 
-      other->_invert_composition_cache.find(this);
-    if (oci != other->_invert_composition_cache.end()) {
-      Composition ocomp = (*oci).second;
-      other->_invert_composition_cache.erase(oci);
+    if (other != this) {
+      CompositionCache::iterator oci = 
+        other->_invert_composition_cache.find(this);
+      if (oci != other->_invert_composition_cache.end()) {
+        Composition ocomp = (*oci).second;
+        other->_invert_composition_cache.erase(oci);
+        if (ocomp._result != (const RenderState *)NULL && ocomp._result != other) {
+          unref_delete(ocomp._result);
+        }
+      }
+    }
+    if (comp._result != (const RenderState *)NULL && comp._result != this) {
+      unref_delete(comp._result);
     }
   }
 
-  // Also, if we called compose(this) at some point and the return
-  // value was something other than this, we need to decrement the
-  // associated reference count.
-  if (_self_compose != (RenderState *)NULL && _self_compose != this) {
-    unref_delete((RenderState *)_self_compose);
-  }
+  // If this was true at the beginning of the destructor, but is no
+  // longer true now, probably we've been double-deleted.
+  nassertv(get_ref_count() == 0);
 }
 
 ////////////////////////////////////////////////////////////////////
 //     Function: RenderState::operator <
-//       Access: Public
+//       Access: Published
 //  Description: Provides an arbitrary ordering among all unique
 //               RenderStates, so we can store the essentially
 //               different ones in a big set and throw away the rest.
@@ -350,28 +357,6 @@ compose(const RenderState *other) const {
     return this;
   }
 
-  if (other == this) {
-    // compose(this) has to be handled as a special case, because the
-    // caching problem is so different.
-    if (_self_compose != (RenderState *)NULL) {
-      return _self_compose;
-    }
-    CPT(RenderState) result = do_compose(this);
-    ((RenderState *)this)->_self_compose = result;
-
-    if (result != (const RenderState *)this) {
-      // If the result of compose(this) is something other than this,
-      // explicitly increment the reference count.  We have to be sure
-      // to decrement it again later, in our destructor.
-      _self_compose->ref();
-
-      // (If the result was just this again, we still store the
-      // result, but we don't increment the reference count, since
-      // that would be a self-referential leak.  What a mess this is.)
-    }
-    return _self_compose;
-  }
-
   // Is this composition already cached?
   CompositionCache::const_iterator ci = _composition_cache.find(other);
   if (ci != _composition_cache.end()) {
@@ -380,7 +365,14 @@ compose(const RenderState *other) const {
       // Well, it wasn't cached already, but we already had an entry
       // (probably created for the reverse direction), so use the same
       // entry to store the new result.
-      ((Composition &)comp)._result = do_compose(other);
+      CPT(RenderState) result = do_compose(other);
+      ((Composition &)comp)._result = result;
+
+      if (result != (const RenderState *)this) {
+        // See the comments below about the need to up the reference
+        // count only when the result is not the same as this.
+        result->ref();
+      }
     }
     // Here's the cache!
     return comp._result;
@@ -395,9 +387,22 @@ compose(const RenderState *other) const {
   // result; the other will be NULL for now.
   CPT(RenderState) result = do_compose(other);
 
+  // Order is important here, in case other == this.
   ((RenderState *)other)->_composition_cache[this]._result = NULL;
   ((RenderState *)this)->_composition_cache[other]._result = result;
 
+  if (result != (const RenderState *)this) {
+    // If the result of compose() is something other than this,
+    // explicitly increment the reference count.  We have to be sure
+    // to decrement it again later, when the composition entry is
+    // removed from the cache.
+    result->ref();
+    
+    // (If the result was just this again, we still store the
+    // result, but we don't increment the reference count, since
+    // that would be a self-referential leak.)
+  }
+
   return result;
 }
 
@@ -438,7 +443,14 @@ invert_compose(const RenderState *other) const {
       // Well, it wasn't cached already, but we already had an entry
       // (probably created for the reverse direction), so use the same
       // entry to store the new result.
-      ((Composition &)comp)._result = do_invert_compose(other);
+      CPT(RenderState) result = do_invert_compose(other);
+      ((Composition &)comp)._result = result;
+
+      if (result != (const RenderState *)this) {
+        // See the comments below about the need to up the reference
+        // count only when the result is not the same as this.
+        result->ref();
+      }
     }
     // Here's the cache!
     return comp._result;
@@ -456,6 +468,18 @@ invert_compose(const RenderState *other) const {
   ((RenderState *)other)->_invert_composition_cache[this]._result = NULL;
   ((RenderState *)this)->_invert_composition_cache[other]._result = result;
 
+  if (result != (const RenderState *)this) {
+    // If the result of compose() is something other than this,
+    // explicitly increment the reference count.  We have to be sure
+    // to decrement it again later, when the composition entry is
+    // removed from the cache.
+    result->ref();
+    
+    // (If the result was just this again, we still store the
+    // result, but we don't increment the reference count, since
+    // that would be a self-referential leak.)
+  }
+
   return result;
 }
 
@@ -658,7 +682,18 @@ get_num_states() {
 //       Access: Published, Static
 //  Description: Returns the total number of RenderState objects that
 //               have been allocated but have no references outside of
-//               the internal RenderState map.
+//               the internal RenderState cache.
+//
+//               A nonzero return value is not necessarily indicative
+//               of leaked references; it is normal for two
+//               RenderState objects, both of which have references
+//               held outside the cache, to have to result of their
+//               composition stored within the cache.  This result
+//               will be retained within the cache until one of the
+//               base RenderStates is released.
+//
+//               Use list_cycles() to get an idea of the number of
+//               actual "leaked" RenderState objects.
 ////////////////////////////////////////////////////////////////////
 int RenderState::
 get_num_unused_states() {
@@ -666,8 +701,6 @@ get_num_unused_states() {
     return 0;
   }
 
-  int num_unused = 0;
-
   // First, we need to count the number of times each RenderState
   // object is recorded in the cache.
   typedef pmap<const RenderState *, int> StateCount;
@@ -682,7 +715,7 @@ get_num_unused_states() {
          ci != state->_composition_cache.end();
          ++ci) {
       const RenderState *result = (*ci).second._result;
-      if (result != (const RenderState *)NULL) {
+      if (result != (const RenderState *)NULL && result != state) {
         // Here's a RenderState that's recorded in the cache.
         // Count it.
         pair<StateCount::iterator, bool> ir =
@@ -698,7 +731,7 @@ get_num_unused_states() {
          ci != state->_invert_composition_cache.end();
          ++ci) {
       const RenderState *result = (*ci).second._result;
-      if (result != (const RenderState *)NULL) {
+      if (result != (const RenderState *)NULL && result != state) {
         pair<StateCount::iterator, bool> ir =
           state_count.insert(StateCount::value_type(result, 1));
         if (!ir.second) {
@@ -706,26 +739,13 @@ get_num_unused_states() {
         }
       }
     }
-
-    // Finally, check the self_compose field, which might be reference
-    // counted too.
-    if (state->_self_compose != (const RenderState *)NULL &&
-        state->_self_compose != state) {
-      const RenderState *result = state->_self_compose;
-      if (result != (const RenderState *)NULL) {
-        pair<StateCount::iterator, bool> ir =
-          state_count.insert(StateCount::value_type(result, 1));
-        if (!ir.second) {
-          (*(ir.first)).second++;
-        }
-      }
-    }
-
   }
 
   // Now that we have the appearance count of each RenderState
   // object, we can tell which ones are unreferenced outside of the
   // RenderState cache, by comparing these to the reference counts.
+  int num_unused = 0;
+
   StateCount::iterator sci;
   for (sci = state_count.begin(); sci != state_count.end(); ++sci) {
     const RenderState *state = (*sci).first;
@@ -733,6 +753,13 @@ get_num_unused_states() {
     nassertr(count <= state->get_ref_count(), num_unused);
     if (count == state->get_ref_count()) {
       num_unused++;
+
+      if (pgraph_cat.is_debug()) {
+        pgraph_cat.debug()
+          << "Unused state: " << (void *)state << ":" 
+          << state->get_ref_count() << " =\n";
+        state->write(pgraph_cat.debug(false), 2);
+      }
     }
   }
 
@@ -790,13 +817,29 @@ clear_cache() {
     TempStates::iterator ti;
     for (ti = temp_states.begin(); ti != temp_states.end(); ++ti) {
       RenderState *state = (RenderState *)(*ti).p();
+
+      CompositionCache::const_iterator ci;
+      for (ci = state->_composition_cache.begin();
+           ci != state->_composition_cache.end();
+           ++ci) {
+        const RenderState *result = (*ci).second._result;
+        if (result != (const RenderState *)NULL && result != state) {
+          result->unref();
+          nassertr(result->get_ref_count() > 0, 0);
+        }
+      }
       state->_composition_cache.clear();
-      state->_invert_composition_cache.clear();
-      if (state->_self_compose != (RenderState *)NULL && 
-          state->_self_compose != state) {
-        unref_delete((RenderState *)state->_self_compose);
-        state->_self_compose = (RenderState *)NULL;
+
+      for (ci = state->_invert_composition_cache.begin();
+           ci != state->_invert_composition_cache.end();
+           ++ci) {
+        const RenderState *result = (*ci).second._result;
+        if (result != (const RenderState *)NULL && result != state) {
+          result->unref();
+          nassertr(result->get_ref_count() > 0, 0);
+        }
       }
+      state->_invert_composition_cache.clear();
     }
 
     // Once this block closes and the temp_states object goes away,
@@ -808,6 +851,120 @@ clear_cache() {
   return orig_size - new_size;
 }
 
+////////////////////////////////////////////////////////////////////
+//     Function: RenderState::list_cycles
+//       Access: Published, Static
+//  Description: Detects all of the reference-count cycles in the
+//               cache and reports them to standard output.
+//
+//               These cycles may be inadvertently created when state
+//               compositions cycle back to a starting point.  In the
+//               current implementation, they are not automatically
+//               detected and broken, so they represent a kind of
+//               memory leak.  They will not be freed unless
+//               clear_cache() is called explicitly.
+//
+//               The cycles listed here are not leaks in the strictest
+//               sense of the word, since they can be reclaimed by a
+//               call to clear_cache(); but they will not be reclaimed
+//               automatically.
+////////////////////////////////////////////////////////////////////
+void RenderState::
+list_cycles(ostream &out) {
+  VisitedStates visited;
+  CompositionCycleDesc cycle_desc;
+
+  States::iterator si;
+  for (si = _states->begin(); si != _states->end(); ++si) {
+    const RenderState *state = (*si);
+
+    bool inserted = visited.insert(state).second;
+    if (inserted) {
+      VisitedStates visited_this_cycle;
+      CycleChain chain(state);
+      if (r_detect_cycles(state, visited_this_cycle, &chain, cycle_desc)) {
+        // This state begins a cycle.
+        CompositionCycleDesc::reverse_iterator csi;
+
+        out << "\nCycle detected of length " << cycle_desc.size() + 1 << ":\n"
+            << "state " << (void *)state << ":" << state->get_ref_count()
+            << " =\n";
+        state->write(out, 2);
+        for (csi = cycle_desc.rbegin(); csi != cycle_desc.rend(); ++csi) {
+          const CompositionCycleDescEntry &entry = (*csi);
+          if (entry._inverted) {
+            out << "invert composed with ";
+          } else {
+            out << "composed with ";
+          }
+          out << (const void *)entry._obj << ":" << entry._obj->get_ref_count()
+              << " " << *entry._obj << "\n"
+              << "produces " << (const void *)entry._result << ":"
+              << entry._result->get_ref_count() << " =\n";
+          entry._result->write(out, 2);
+          visited.insert(entry._result);
+        }
+
+        cycle_desc.clear();
+      }
+    }
+  }
+}
+
+
+////////////////////////////////////////////////////////////////////
+//     Function: RenderState::list_states
+//       Access: Published, Static
+//  Description: Lists all of the RenderStates in the cache to the
+//               output stream, one per line.  This can be quite a lot
+//               of output if the cache is large, so be prepared.
+////////////////////////////////////////////////////////////////////
+void RenderState::
+list_states(ostream &out) {
+  out << _states->size() << " states:\n";
+  States::const_iterator si;
+  for (si = _states->begin(); si != _states->end(); ++si) {
+    const RenderState *state = (*si);
+    state->write(out, 2);
+  }
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: RenderState::validate_states
+//       Access: Published, Static
+//  Description: Ensures that the cache is still stored in sorted
+//               order, and that none of the cache elements have been
+//               inadvertently deleted.  Returns true if so, false if
+//               there is a problem (which implies someone has
+//               modified one of the supposedly-const RenderState
+//               objects).
+////////////////////////////////////////////////////////////////////
+bool RenderState::
+validate_states() {
+  if (_states->empty()) {
+    return true;
+  }
+
+  States::const_iterator si = _states->begin();
+  States::const_iterator snext = si;
+  ++snext;
+  nassertr((*si)->get_ref_count() > 0, false);
+  while (snext != _states->end()) {
+    if (!(*(*si) < *(*snext))) {
+      pgraph_cat.error()
+        << "RenderStates out of order!\n";
+      (*si)->write(pgraph_cat.error(false), 2);
+      (*snext)->write(pgraph_cat.error(false), 2);
+      return false;
+    }
+    si = snext;
+    ++snext;
+    nassertr((*si)->get_ref_count() > 0, false);
+  }
+
+  return true;
+}
+
 ////////////////////////////////////////////////////////////////////
 //     Function: RenderState::issue_delta_modify
 //       Access: Public
@@ -982,6 +1139,12 @@ return_new(RenderState *state) {
   // for anything else.
   nassertr(state->_saved_entry == _states->end(), state);
 
+#ifndef NDEBUG
+  if (paranoid_const) {
+    nassertr(validate_states(), state);
+  }
+#endif
+
   // Save the state in a local PointerTo so that it will be freed at
   // the end of this function if no one else uses it.
   CPT(RenderState) pt_state = state;
@@ -1120,6 +1283,67 @@ do_invert_compose(const RenderState *other) const {
   return return_new(new_state);
 }
 
+////////////////////////////////////////////////////////////////////
+//     Function: RenderState::r_detect_cycles
+//       Access: Private, Static
+//  Description: Detects whether there is a cycle in the cache that
+//               begins with the indicated state.  Returns true if at
+//               least one cycle is found, false if this state is not
+//               part of any cycles.  If a cycle is found, cycle_desc
+//               is filled in with the list of the steps of the cycle,
+//               in reverse order.
+////////////////////////////////////////////////////////////////////
+bool RenderState::
+r_detect_cycles(const RenderState *state,
+                RenderState::VisitedStates &visited_this_cycle,
+                RenderState::CycleChain *chain,
+                RenderState::CompositionCycleDesc &cycle_desc) {
+  bool inserted = visited_this_cycle.insert(state).second;
+  if (!inserted && chain->has_result(state)) {
+    // We've already seen this state; therefore, we've found a cycle.
+
+    // However, we only care about cycles that involve more than two
+    // steps.  If only one or two nodes are involved, it doesn't
+    // represent a memory leak, so no problem there.
+    return (chain->_length > 2);
+  }
+    
+  CompositionCache::const_iterator ci;
+  for (ci = state->_composition_cache.begin();
+       ci != state->_composition_cache.end();
+       ++ci) {
+    const RenderState *result = (*ci).second._result;
+    if (result != (const RenderState *)NULL) {
+      CycleChain next_chain(chain, result);
+      if (r_detect_cycles(result, visited_this_cycle, &next_chain, cycle_desc)) {
+        // Cycle detected.
+        CompositionCycleDescEntry entry((*ci).first, result, false);
+        cycle_desc.push_back(entry);
+        return true;
+      }
+    }
+  }
+
+  for (ci = state->_invert_composition_cache.begin();
+       ci != state->_invert_composition_cache.end();
+       ++ci) {
+    const RenderState *result = (*ci).second._result;
+    if (result != (const RenderState *)NULL) {
+      CycleChain next_chain(chain, result);
+      if (r_detect_cycles(result, visited_this_cycle, &next_chain, cycle_desc)) {
+        // Cycle detected.
+        CompositionCycleDescEntry entry((*ci).first, result, true);
+        cycle_desc.push_back(entry);
+        return true;
+      }
+    }
+  }
+
+  // No cycle detected.
+  return false;
+}
+
+
 ////////////////////////////////////////////////////////////////////
 //     Function: RenderState::determine_bin_index
 //       Access: Private
@@ -1375,3 +1599,20 @@ fillin(DatagramIterator &scan, BamReader *manager) {
     _attributes.push_back(Attribute(override));
   }
 }
+
+////////////////////////////////////////////////////////////////////
+//     Function: RenderState::CycleChain::has_result
+//       Access: Public
+//  Description: Returns true if the indicated state has been reached
+//               in this chain previously, false otherwise.
+////////////////////////////////////////////////////////////////////
+bool RenderState::CycleChain::
+has_result(const RenderState *state) const {
+  if (_state == state) {
+    return true;
+  }
+  if (_prev != NULL) {
+    return _prev->has_result(state);
+  }
+  return false;
+}

panda/src/pgraph/renderState.h

@@ -56,9 +56,9 @@ private:
 public:
   virtual ~RenderState();
 
+PUBLISHED:
   bool operator < (const RenderState &other) const;
 
-PUBLISHED:
   INLINE bool is_empty() const;
   INLINE int get_num_attribs() const;
   INLINE const RenderAttrib *get_attrib(int n) const;
@@ -99,6 +99,9 @@ PUBLISHED:
   static int get_num_states();
   static int get_num_unused_states();
   static int clear_cache();
+  static void list_cycles(ostream &out);
+  static void list_states(ostream &out);
+  static bool validate_states();
 
 PUBLISHED:
   // These methods are intended for use by low-level code, but they're
@@ -118,9 +121,38 @@ public:
   static void bin_removed(int bin_index);
 
 private:
+  class CompositionCycleDescEntry {
+  public:
+    INLINE CompositionCycleDescEntry(const RenderState *obj,
+                                     const RenderState *result,
+                                     bool inverted);
+
+    const RenderState *_obj;
+    const RenderState *_result;
+    bool _inverted;
+  };
+  typedef pvector<CompositionCycleDescEntry> CompositionCycleDesc;
+  typedef pset<const RenderState *> VisitedStates;
+  class CycleChain {
+  public:
+    INLINE CycleChain(const RenderState *state);
+    INLINE CycleChain(CycleChain *prev, const RenderState *state);
+
+    bool has_result(const RenderState *state) const;
+
+    const RenderState *_state;
+    CycleChain *_prev;
+    int _length;
+  };
+
   static CPT(RenderState) return_new(RenderState *state);
   CPT(RenderState) do_compose(const RenderState *other) const;
   CPT(RenderState) do_invert_compose(const RenderState *other) const;
+  static bool r_detect_cycles(const RenderState *state,
+                              VisitedStates &visited_this_cycle,
+                              CycleChain *chain,
+                              CompositionCycleDesc &cycle_desc);
+
   void determine_bin_index();
   void determine_fog();
   void determine_bin();
@@ -149,21 +181,19 @@ private:
     INLINE Composition();
     INLINE Composition(const Composition &copy);
 
-    CPT(RenderState) _result;
+    // _result is reference counted if and only if it is not the same
+    // pointer as this.
+    const RenderState *_result;
   };
-    
+
+  // The first element of the map is the object we compose with.  This
+  // is not reference counted within this map; instead we store a
+  // companion pointer in the other object, and remove the references
+  // explicitly when either object destructs.
   typedef pmap<const RenderState *, Composition> CompositionCache;
   CompositionCache _composition_cache;
   CompositionCache _invert_composition_cache;
 
-  // Thise pointer is used to cache the result of compose(this).  This
-  // has to be a special case, because we have to handle the reference
-  // counts carefully so that we don't leak.  Most of the time, the
-  // result of compose(this) is this, which should not be reference
-  // counted, but other times the result is something else (which
-  // should be).
-  const RenderState *_self_compose;
-
 private:
   // This is the actual data within the RenderState: a set of
   // RenderAttribs.

panda/src/pgraph/transformState.I

@@ -17,26 +17,6 @@
 ////////////////////////////////////////////////////////////////////
 
 
-////////////////////////////////////////////////////////////////////
-//     Function: TransformState::Composition::Constructor
-//       Access: Public
-//  Description: 
-////////////////////////////////////////////////////////////////////
-INLINE TransformState::Composition::
-Composition() {
-}
-
-////////////////////////////////////////////////////////////////////
-//     Function: TransformState::Composition::Copy Constructor
-//       Access: Public
-//  Description: 
-////////////////////////////////////////////////////////////////////
-INLINE TransformState::Composition::
-Composition(const TransformState::Composition &copy) :
-  _result(copy._result)
-{
-}
-
 ////////////////////////////////////////////////////////////////////
 //     Function: TransformState::make_pos
 //       Access: Published, Static
@@ -582,3 +562,64 @@ is_destructing() const {
   return false;
 #endif
 }
+
+////////////////////////////////////////////////////////////////////
+//     Function: TransformState::Composition::Constructor
+//       Access: Public
+//  Description: 
+////////////////////////////////////////////////////////////////////
+INLINE TransformState::Composition::
+Composition() {
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: TransformState::Composition::Copy Constructor
+//       Access: Public
+//  Description: 
+////////////////////////////////////////////////////////////////////
+INLINE TransformState::Composition::
+Composition(const TransformState::Composition &copy) :
+  _result(copy._result)
+{
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: TransformState::CompositionCycleDescEntry::Constructor
+//       Access: Public
+//  Description: 
+////////////////////////////////////////////////////////////////////
+INLINE TransformState::CompositionCycleDescEntry::
+CompositionCycleDescEntry(const TransformState *obj,
+                          const TransformState *result,
+                          bool inverted) :
+  _obj(obj),
+  _result(result),
+  _inverted(inverted)
+{
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: TransformState::CycleChain::Constructor
+//       Access: Public
+//  Description: 
+////////////////////////////////////////////////////////////////////
+INLINE TransformState::CycleChain::
+CycleChain(const TransformState *state) :
+  _state(state),
+  _prev(NULL),
+  _length(1)
+{
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: TransformState::CycleChain::Constructor
+//       Access: Public
+//  Description: 
+////////////////////////////////////////////////////////////////////
+INLINE TransformState::CycleChain::
+CycleChain(CycleChain *prev, const TransformState *state) :
+  _state(state),
+  _prev(prev),
+  _length(prev->_length + 1)
+{
+}

panda/src/pgraph/transformState.cxx

@@ -47,7 +47,6 @@ TransformState() {
     _states = new States;
   }
   _saved_entry = _states->end();
-  _self_compose = (TransformState *)NULL;
   _flags = F_is_identity | F_singular_known;
   _inv_mat = (LMatrix4f *)NULL;
 }
@@ -83,14 +82,13 @@ TransformState::
   // We'd better not call the destructor twice on a particular object.
   nassertv(!is_destructing());
   set_destructing();
-
+ 
   // Free the inverse matrix computation, if it has been stored.
   if (_inv_mat != (LMatrix4f *)NULL) {
     delete _inv_mat;
     _inv_mat = (LMatrix4f *)NULL;
   }
 
-  // Remove the deleted TransformState object from the global pool.
   if (_saved_entry != _states->end()) {
     nassertv(_states->find(this) == _saved_entry);
     _states->erase(_saved_entry);
@@ -130,13 +128,8 @@ TransformState::
     // objects destruct, there will be no reason for that one to.
     TransformState *other = (TransformState *)(*ci).first;
 
-    // We should never have a reflexive entry in this map.  If we
-    // do, something got screwed up elsewhere.
-    nassertv(other != this);
-
-    // We hold a copy of the composition result to ensure that the
-    // result TransformState object (if there is one) doesn't
-    // destruct.
+    // We hold a copy of the composition result so we can dereference
+    // it later.
     Composition comp = (*ci).second;
 
     // Now we can remove the element from our cache.  We do this now,
@@ -145,25 +138,33 @@ TransformState::
     // is still valid.
     _composition_cache.erase(ci);
 
-    CompositionCache::iterator oci = other->_composition_cache.find(this);
-
-    // We may or may not still be listed in the other's cache (it
-    // might be halfway through pulling entries out, from within its
-    // own destructor).
-    if (oci != other->_composition_cache.end()) {
-      // Hold a copy of the other composition result, too.
-      Composition ocomp = (*oci).second;
-      
-      // Now we're holding a reference count to both computed
-      // results, so no objects will be tempted to destruct while we
-      // erase the other cache entry.
-      other->_composition_cache.erase(oci);
+    if (other != this) {
+      CompositionCache::iterator oci = other->_composition_cache.find(this);
+
+      // We may or may not still be listed in the other's cache (it
+      // might be halfway through pulling entries out, from within its
+      // own destructor).
+      if (oci != other->_composition_cache.end()) {
+        // Hold a copy of the other composition result, too.
+        Composition ocomp = (*oci).second;
+        
+        other->_composition_cache.erase(oci);
+        
+        // It's finally safe to let our held pointers go away.  This may
+        // have cascading effects as other TransformState objects are
+        // destructed, but there will be no harm done if they destruct
+        // now.
+        if (ocomp._result != (const TransformState *)NULL && ocomp._result != other) {
+          unref_delete(ocomp._result);
+        }
+      }
     }
 
-    // It's finally safe to let our held pointers go away.  This may
-    // have cascading effects as other TransformState objects are
-    // destructed, but there will be no harm done if they destruct
-    // now.
+    // It's finally safe to let our held pointers go away.  (See
+    // comment above.)
+    if (comp._result != (const TransformState *)NULL && comp._result != this) {
+      unref_delete(comp._result);
+    }
   }
 
   // A similar bit of code for the invert cache.
@@ -173,19 +174,20 @@ TransformState::
     nassertv(other != this);
     Composition comp = (*ci).second;
     _invert_composition_cache.erase(ci);
-    CompositionCache::iterator oci = 
-      other->_invert_composition_cache.find(this);
-    if (oci != other->_invert_composition_cache.end()) {
-      Composition ocomp = (*oci).second;
-      other->_invert_composition_cache.erase(oci);
+    if (other != this) {
+      CompositionCache::iterator oci = 
+        other->_invert_composition_cache.find(this);
+      if (oci != other->_invert_composition_cache.end()) {
+        Composition ocomp = (*oci).second;
+        other->_invert_composition_cache.erase(oci);
+        if (ocomp._result != (const TransformState *)NULL && ocomp._result != other) {
+          unref_delete(ocomp._result);
+        }
+      }
+    }
+    if (comp._result != (const TransformState *)NULL && comp._result != this) {
+      unref_delete(comp._result);
     }
-  }
-
-  // Also, if we called compose(this) at some point and the return
-  // value was something other than this, we need to decrement the
-  // associated reference count.
-  if (_self_compose != (TransformState *)NULL && _self_compose != this) {
-    unref_delete((TransformState *)_self_compose);
   }
 
   // If this was true at the beginning of the destructor, but is no
@@ -195,7 +197,7 @@ TransformState::
 
 ////////////////////////////////////////////////////////////////////
 //     Function: TransformState::operator <
-//       Access: Public
+//       Access: Published
 //  Description: Provides an arbitrary ordering among all unique
 //               TransformStates, so we can store the essentially
 //               different ones in a big set and throw away the rest.
@@ -483,7 +485,7 @@ compose(const TransformState *other) const {
   if (other->is_identity()) {
     return this;
   }
-
+ 
   // If either transform is invalid, the result is invalid.
   if (is_invalid()) {
     return this;
@@ -492,28 +494,6 @@ compose(const TransformState *other) const {
     return other;
   }
 
-  if (other == this) {
-    // compose(this) has to be handled as a special case, because the
-    // caching problem is so different.
-    if (_self_compose != (TransformState *)NULL) {
-      return _self_compose;
-    }
-    CPT(TransformState) result = do_compose(this);
-    ((TransformState *)this)->_self_compose = result;
-
-    if (result != (const TransformState *)this) {
-      // If the result of compose(this) is something other than this,
-      // explicitly increment the reference count.  We have to be sure
-      // to decrement it again later, in our destructor.
-      _self_compose->ref();
-
-      // (If the result was just this again, we still store the
-      // result, but we don't increment the reference count, since
-      // that would be a self-referential leak.  What a mess this is.)
-    }
-    return _self_compose;
-  }
-
   // Is this composition already cached?
   CompositionCache::const_iterator ci = _composition_cache.find(other);
   if (ci != _composition_cache.end()) {
@@ -522,7 +502,14 @@ compose(const TransformState *other) const {
       // Well, it wasn't cached already, but we already had an entry
       // (probably created for the reverse direction), so use the same
       // entry to store the new result.
-      ((Composition &)comp)._result = do_compose(other);
+      CPT(TransformState) result = do_compose(other);
+      ((Composition &)comp)._result = result;
+
+      if (result != (const TransformState *)this) {
+        // See the comments below about the need to up the reference
+        // count only when the result is not the same as this.
+        result->ref();
+      }
     }
     // Here's the cache!
     return comp._result;
@@ -537,9 +524,22 @@ compose(const TransformState *other) const {
   // result; the other will be NULL for now.
   CPT(TransformState) result = do_compose(other);
 
+  // Order is important here, in case other == this.
   ((TransformState *)other)->_composition_cache[this]._result = NULL;
   ((TransformState *)this)->_composition_cache[other]._result = result;
 
+  if (result != (const TransformState *)this) {
+    // If the result of compose() is something other than this,
+    // explicitly increment the reference count.  We have to be sure
+    // to decrement it again later, when the composition entry is
+    // removed from the cache.
+    result->ref();
+    
+    // (If the result was just this again, we still store the
+    // result, but we don't increment the reference count, since
+    // that would be a self-referential leak.)
+  }
+
   return result;
 }
 
@@ -588,7 +588,14 @@ invert_compose(const TransformState *other) const {
       // Well, it wasn't cached already, but we already had an entry
       // (probably created for the reverse direction), so use the same
       // entry to store the new result.
-      ((Composition &)comp)._result = do_invert_compose(other);
+      CPT(TransformState) result = do_invert_compose(other);
+      ((Composition &)comp)._result = result;
+
+      if (result != (const TransformState *)this) {
+        // See the comments below about the need to up the reference
+        // count only when the result is not the same as this.
+        result->ref();
+      }
     }
     // Here's the cache!
     return comp._result;
@@ -606,6 +613,18 @@ invert_compose(const TransformState *other) const {
   ((TransformState *)other)->_invert_composition_cache[this]._result = NULL;
   ((TransformState *)this)->_invert_composition_cache[other]._result = result;
 
+  if (result != (const TransformState *)this) {
+    // If the result of compose() is something other than this,
+    // explicitly increment the reference count.  We have to be sure
+    // to decrement it again later, when the composition entry is
+    // removed from the cache.
+    result->ref();
+    
+    // (If the result was just this again, we still store the
+    // result, but we don't increment the reference count, since
+    // that would be a self-referential leak.)
+  }
+
   return result;
 }
 
@@ -702,9 +721,20 @@ get_num_states() {
 ////////////////////////////////////////////////////////////////////
 //     Function: TransformState::get_num_unused_states
 //       Access: Published, Static
-//  Description: Returns the total number of TransformState objects
-//               that have been allocated but have no references
-//               outside of the internal TransformState map.
+//  Description: Returns the total number of TransformState objects that
+//               have been allocated but have no references outside of
+//               the internal TransformState cache.
+//
+//               A nonzero return value is not necessarily indicative
+//               of leaked references; it is normal for two
+//               TransformState objects, both of which have references
+//               held outside the cache, to have to result of their
+//               composition stored within the cache.  This result
+//               will be retained within the cache until one of the
+//               base TransformStates is released.
+//
+//               Use list_cycles() to get an idea of the number of
+//               actual "leaked" TransformState objects.
 ////////////////////////////////////////////////////////////////////
 int TransformState::
 get_num_unused_states() {
@@ -712,8 +742,6 @@ get_num_unused_states() {
     return 0;
   }
 
-  int num_unused = 0;
-
   // First, we need to count the number of times each TransformState
   // object is recorded in the cache.
   typedef pmap<const TransformState *, int> StateCount;
@@ -728,7 +756,7 @@ get_num_unused_states() {
          ci != state->_composition_cache.end();
          ++ci) {
       const TransformState *result = (*ci).second._result;
-      if (result != (const TransformState *)NULL) {
+      if (result != (const TransformState *)NULL && result != state) {
         // Here's a TransformState that's recorded in the cache.
         // Count it.
         pair<StateCount::iterator, bool> ir =
@@ -744,7 +772,7 @@ get_num_unused_states() {
          ci != state->_invert_composition_cache.end();
          ++ci) {
       const TransformState *result = (*ci).second._result;
-      if (result != (const TransformState *)NULL) {
+      if (result != (const TransformState *)NULL && result != state) {
         pair<StateCount::iterator, bool> ir =
           state_count.insert(StateCount::value_type(result, 1));
         if (!ir.second) {
@@ -752,26 +780,13 @@ get_num_unused_states() {
         }
       }
     }
-
-    // Finally, check the self_compose field, which might be reference
-    // counted too.
-    if (state->_self_compose != (const TransformState *)NULL &&
-        state->_self_compose != state) {
-      const TransformState *result = state->_self_compose;
-      if (result != (const TransformState *)NULL) {
-        pair<StateCount::iterator, bool> ir =
-          state_count.insert(StateCount::value_type(result, 1));
-        if (!ir.second) {
-          (*(ir.first)).second++;
-        }
-      }
-    }
-
   }
 
   // Now that we have the appearance count of each TransformState
   // object, we can tell which ones are unreferenced outside of the
   // TransformState cache, by comparing these to the reference counts.
+  int num_unused = 0;
+
   StateCount::iterator sci;
   for (sci = state_count.begin(); sci != state_count.end(); ++sci) {
     const TransformState *state = (*sci).first;
@@ -779,6 +794,13 @@ get_num_unused_states() {
     nassertr(count <= state->get_ref_count(), num_unused);
     if (count == state->get_ref_count()) {
       num_unused++;
+
+      if (pgraph_cat.is_debug()) {
+        pgraph_cat.debug()
+          << "Unused state: " << (void *)state << ":" 
+          << state->get_ref_count() << " =\n";
+        state->write(pgraph_cat.debug(false), 2);
+      }
     }
   }
 
@@ -818,10 +840,10 @@ clear_cache() {
 
   int orig_size = _states->size();
 
-  // First, we need to copy the entire set of transforms to a
-  // temporary vector, reference-counting each object.  That way we
-  // can walk through the copy, without fear of dereferencing (and
-  // deleting) the objects in the map as we go.
+  // First, we need to copy the entire set of states to a temporary
+  // vector, reference-counting each object.  That way we can walk
+  // through the copy, without fear of dereferencing (and deleting)
+  // the objects in the map as we go.
   {
     typedef pvector< CPT(TransformState) > TempStates;
     TempStates temp_states;
@@ -836,13 +858,29 @@ clear_cache() {
     TempStates::iterator ti;
     for (ti = temp_states.begin(); ti != temp_states.end(); ++ti) {
       TransformState *state = (TransformState *)(*ti).p();
+
+      CompositionCache::const_iterator ci;
+      for (ci = state->_composition_cache.begin();
+           ci != state->_composition_cache.end();
+           ++ci) {
+        const TransformState *result = (*ci).second._result;
+        if (result != (const TransformState *)NULL && result != state) {
+          result->unref();
+          nassertr(result->get_ref_count() > 0, 0);
+        }
+      }
       state->_composition_cache.clear();
-      state->_invert_composition_cache.clear();
-      if (state->_self_compose != (TransformState *)NULL && 
-          state->_self_compose != state) {
-        unref_delete((TransformState *)state->_self_compose);
-        state->_self_compose = (TransformState *)NULL;
+
+      for (ci = state->_invert_composition_cache.begin();
+           ci != state->_invert_composition_cache.end();
+           ++ci) {
+        const TransformState *result = (*ci).second._result;
+        if (result != (const TransformState *)NULL && result != state) {
+          result->unref();
+          nassertr(result->get_ref_count() > 0, 0);
+        }
       }
+      state->_invert_composition_cache.clear();
     }
 
     // Once this block closes and the temp_states object goes away,
@@ -854,6 +892,120 @@ clear_cache() {
   return orig_size - new_size;
 }
 
+////////////////////////////////////////////////////////////////////
+//     Function: TransformState::list_cycles
+//       Access: Published, Static
+//  Description: Detects all of the reference-count cycles in the
+//               cache and reports them to standard output.
+//
+//               These cycles may be inadvertently created when state
+//               compositions cycle back to a starting point.  In the
+//               current implementation, they are not automatically
+//               detected and broken, so they represent a kind of
+//               memory leak.  They will not be freed unless
+//               clear_cache() is called explicitly.
+//
+//               The cycles listed here are not leaks in the strictest
+//               sense of the word, since they can be reclaimed by a
+//               call to clear_cache(); but they will not be reclaimed
+//               automatically.
+////////////////////////////////////////////////////////////////////
+void TransformState::
+list_cycles(ostream &out) {
+  VisitedStates visited;
+  CompositionCycleDesc cycle_desc;
+
+  States::iterator si;
+  for (si = _states->begin(); si != _states->end(); ++si) {
+    const TransformState *state = (*si);
+
+    bool inserted = visited.insert(state).second;
+    if (inserted) {
+      VisitedStates visited_this_cycle;
+      CycleChain chain(state);
+      if (r_detect_cycles(state, visited_this_cycle, &chain, cycle_desc)) {
+        // This state begins a cycle.
+        CompositionCycleDesc::reverse_iterator csi;
+
+        out << "\nCycle detected of length " << cycle_desc.size() + 1 << ":\n"
+            << "state " << (void *)state << ":" << state->get_ref_count()
+            << " =\n";
+        state->write(out, 2);
+        for (csi = cycle_desc.rbegin(); csi != cycle_desc.rend(); ++csi) {
+          const CompositionCycleDescEntry &entry = (*csi);
+          if (entry._inverted) {
+            out << "invert composed with ";
+          } else {
+            out << "composed with ";
+          }
+          out << (const void *)entry._obj << ":" << entry._obj->get_ref_count()
+              << " " << *entry._obj << "\n"
+              << "produces " << (const void *)entry._result << ":"
+              << entry._result->get_ref_count() << " =\n";
+          entry._result->write(out, 2);
+          visited.insert(entry._result);
+        }
+
+        cycle_desc.clear();
+      }
+    }
+  }
+}
+
+
+////////////////////////////////////////////////////////////////////
+//     Function: TransformState::list_states
+//       Access: Published, Static
+//  Description: Lists all of the TransformStates in the cache to the
+//               output stream, one per line.  This can be quite a lot
+//               of output if the cache is large, so be prepared.
+////////////////////////////////////////////////////////////////////
+void TransformState::
+list_states(ostream &out) {
+  out << _states->size() << " states:\n";
+  States::const_iterator si;
+  for (si = _states->begin(); si != _states->end(); ++si) {
+    const TransformState *state = (*si);
+    state->write(out, 2);
+  }
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: TransformState::validate_states
+//       Access: Published, Static
+//  Description: Ensures that the cache is still stored in sorted
+//               order, and that none of the cache elements have been
+//               inadvertently deleted.  Returns true if so, false if
+//               there is a problem (which implies someone has
+//               modified one of the supposedly-const TransformState
+//               objects).
+////////////////////////////////////////////////////////////////////
+bool TransformState::
+validate_states() {
+  if (_states->empty()) {
+    return true;
+  }
+
+  States::const_iterator si = _states->begin();
+  States::const_iterator snext = si;
+  ++snext;
+  nassertr((*si)->get_ref_count() > 0, false);
+  while (snext != _states->end()) {
+    if (!(*(*si) < *(*snext))) {
+      pgraph_cat.error()
+        << "TransformStates out of order!\n";
+      (*si)->write(pgraph_cat.error(false), 2);
+      (*snext)->write(pgraph_cat.error(false), 2);
+      return false;
+    }
+    si = snext;
+    ++snext;
+    nassertr((*si)->get_ref_count() > 0, false);
+  }
+
+  return true;
+}
+
 ////////////////////////////////////////////////////////////////////
 //     Function: TransformState::return_new
 //       Access: Private, Static
@@ -874,6 +1026,12 @@ return_new(TransformState *state) {
   // for anything else.
   nassertr(state->_saved_entry == _states->end(), state);
 
+#ifndef NDEBUG
+  if (paranoid_const) {
+    nassertr(validate_states(), state);
+  }
+#endif
+
   // Save the state in a local PointerTo so that it will be freed at
   // the end of this function if no one else uses it.
   CPT(TransformState) pt_state = state;
@@ -1030,6 +1188,66 @@ do_invert_compose(const TransformState *other) const {
   }
 }
 
+////////////////////////////////////////////////////////////////////
+//     Function: TransformState::r_detect_cycles
+//       Access: Private, Static
+//  Description: Detects whether there is a cycle in the cache that
+//               begins with the indicated state.  Returns true if at
+//               least one cycle is found, false if this state is not
+//               part of any cycles.  If a cycle is found, cycle_desc
+//               is filled in with the list of the steps of the cycle,
+//               in reverse order.
+////////////////////////////////////////////////////////////////////
+bool TransformState::
+r_detect_cycles(const TransformState *state,
+                TransformState::VisitedStates &visited_this_cycle,
+                TransformState::CycleChain *chain,
+                TransformState::CompositionCycleDesc &cycle_desc) {
+  bool inserted = visited_this_cycle.insert(state).second;
+  if (!inserted && chain->has_result(state)) {
+    // We've already seen this state; therefore, we've found a cycle.
+
+    // However, we only care about cycles that involve more than two
+    // steps.  If only one or two nodes are involved, it doesn't
+    // represent a memory leak, so no problem there.
+    return (chain->_length > 2);
+  }
+    
+  CompositionCache::const_iterator ci;
+  for (ci = state->_composition_cache.begin();
+       ci != state->_composition_cache.end();
+       ++ci) {
+    const TransformState *result = (*ci).second._result;
+    if (result != (const TransformState *)NULL) {
+      CycleChain next_chain(chain, result);
+      if (r_detect_cycles(result, visited_this_cycle, &next_chain, cycle_desc)) {
+        // Cycle detected.
+        CompositionCycleDescEntry entry((*ci).first, result, false);
+        cycle_desc.push_back(entry);
+        return true;
+      }
+    }
+  }
+
+  for (ci = state->_invert_composition_cache.begin();
+       ci != state->_invert_composition_cache.end();
+       ++ci) {
+    const TransformState *result = (*ci).second._result;
+    if (result != (const TransformState *)NULL) {
+      CycleChain next_chain(chain, result);
+      if (r_detect_cycles(result, visited_this_cycle, &next_chain, cycle_desc)) {
+        // Cycle detected.
+        CompositionCycleDescEntry entry((*ci).first, result, true);
+        cycle_desc.push_back(entry);
+        return true;
+      }
+    }
+  }
+
+  // No cycle detected.
+  return false;
+}
+
 ////////////////////////////////////////////////////////////////////
 //     Function: TransformState::calc_singular
 //       Access: Private
@@ -1326,3 +1544,20 @@ fillin(DatagramIterator &scan, BamReader *manager) {
     _mat.read_datagram(scan);
   }
 }
+
+////////////////////////////////////////////////////////////////////
+//     Function: TransformState::CycleChain::has_result
+//       Access: Public
+//  Description: Returns true if the indicated state has been reached
+//               in this chain previously, false otherwise.
+////////////////////////////////////////////////////////////////////
+bool TransformState::CycleChain::
+has_result(const TransformState *state) const {
+  if (_state == state) {
+    return true;
+  }
+  if (_prev != NULL) {
+    return _prev->has_result(state);
+  }
+  return false;
+}

panda/src/pgraph/transformState.h

@@ -64,9 +64,9 @@ private:
 public:
   virtual ~TransformState();
 
+PUBLISHED:
   bool operator < (const TransformState &other) const;
 
-PUBLISHED:
   static CPT(TransformState) make_identity();
   static CPT(TransformState) make_invalid();
   INLINE static CPT(TransformState) make_pos(const LVecBase3f &pos);
@@ -131,11 +131,42 @@ PUBLISHED:
   static int get_num_states();
   static int get_num_unused_states();
   static int clear_cache();
+  static void list_cycles(ostream &out);
+  static void list_states(ostream &out);
+  static bool validate_states();
 
 private:
+  class CompositionCycleDescEntry {
+  public:
+    INLINE CompositionCycleDescEntry(const TransformState *obj,
+                                     const TransformState *result,
+                                     bool inverted);
+
+    const TransformState *_obj;
+    const TransformState *_result;
+    bool _inverted;
+  };
+  typedef pvector<CompositionCycleDescEntry> CompositionCycleDesc;
+  typedef pset<const TransformState *> VisitedStates;
+  class CycleChain {
+  public:
+    INLINE CycleChain(const TransformState *state);
+    INLINE CycleChain(CycleChain *prev, const TransformState *state);
+
+    bool has_result(const TransformState *state) const;
+
+    const TransformState *_state;
+    CycleChain *_prev;
+    int _length;
+  };
+
   static CPT(TransformState) return_new(TransformState *state);
   CPT(TransformState) do_compose(const TransformState *other) const;
   CPT(TransformState) do_invert_compose(const TransformState *other) const;
+  static bool r_detect_cycles(const TransformState *state,
+                              VisitedStates &visited_this_cycle,
+                              CycleChain *chain,
+                              CompositionCycleDesc &cycle_desc);
 
 private:
   typedef pset<const TransformState *, IndirectLess<TransformState> > States;
@@ -157,18 +188,19 @@ private:
     INLINE Composition();
     INLINE Composition(const Composition &copy);
 
-    CPT(TransformState) _result;
+    // _result is reference counted if and only if it is not the same
+    // pointer as this.
+    const TransformState *_result;
   };
     
+  // The first element of the map is the object we compose with.  This
+  // is not reference counted within this map; instead we store a
+  // companion pointer in the other object, and remove the references
+  // explicitly when either object destructs.
   typedef pmap<const TransformState *, Composition> CompositionCache;
   CompositionCache _composition_cache;
   CompositionCache _invert_composition_cache;
 
-  // Thise pointer is used to cache the result of compose(this).  This
-  // has to be a special case, because we have to handle the reference
-  // counts carefully so that we don't leak.
-  const TransformState *_self_compose;
-
 private:
   // This is the actual data within the TransformState.
   INLINE void check_singular() const;