shadows: fix light buffer not being destructed after light removal

rdb · rdb · commit 9a40febdb9ce · 2017-07-09T20:36:13.000+02:00
Fixes LP 1672089
diff --git a/panda/src/pgraph/light.cxx b/panda/src/pgraph/light.cxx
@@ -157,6 +157,20 @@ get_attenuation() const {
   return no_atten;
 }
 
+/**
+ * This is called when the light is added to a LightAttrib.
+ */
+void Light::
+attrib_ref() {
+}
+
+/**
+ * This is called when the light is removed from a LightAttrib.
+ */
+void Light::
+attrib_unref() {
+}
+
 /**
  * Computes the vector from a particular vertex to this light.  The exact
  * vector depends on the type of light (e.g.  point lights return a different
diff --git a/panda/src/pgraph/light.h b/panda/src/pgraph/light.h
@@ -64,6 +64,9 @@ class EXPCL_PANDA_PGRAPH Light {
   MAKE_PROPERTY(priority, get_priority, set_priority);
 
 public:
+  virtual void attrib_ref();
+  virtual void attrib_unref();
+
   virtual void output(ostream &out) const=0;
   virtual void write(ostream &out, int indent_level) const=0;
   virtual void bind(GraphicsStateGuardianBase *gsg, const NodePath &light,
diff --git a/panda/src/pgraph/lightAttrib.I b/panda/src/pgraph/lightAttrib.I
@@ -18,19 +18,6 @@ INLINE LightAttrib::
 LightAttrib() : _off_all_lights(false), _num_non_ambient_lights(0) {
 }
 
-/**
- * Use LightAttrib::make() to construct a new LightAttrib object.  The copy
- * constructor is only defined to facilitate methods like add_on_light().
- */
-INLINE LightAttrib::
-LightAttrib(const LightAttrib &copy) :
-  _on_lights(copy._on_lights),
-  _off_lights(copy._off_lights),
-  _off_all_lights(copy._off_all_lights),
-  _sort_seq(UpdateSeq::old())
-{
-}
-
 /**
  * Returns the number of lights that are turned on by the attribute.
  */
diff --git a/panda/src/pgraph/lightAttrib.cxx b/panda/src/pgraph/lightAttrib.cxx
@@ -47,6 +47,44 @@ class CompareLightPriorities {
   }
 };
 
+/**
+ * Use LightAttrib::make() to construct a new LightAttrib object.  The copy
+ * constructor is only defined to facilitate methods like add_on_light().
+ */
+LightAttrib::
+LightAttrib(const LightAttrib &copy) :
+  _on_lights(copy._on_lights),
+  _off_lights(copy._off_lights),
+  _off_all_lights(copy._off_all_lights),
+  _sort_seq(UpdateSeq::old())
+{
+  // Increase the attrib_ref of all the lights in this attribute.
+  Lights::const_iterator it;
+  for (it = _on_lights.begin(); it != _on_lights.end(); ++it) {
+    Light *lobj = (*it).node()->as_light();
+    nassertd(lobj != nullptr) continue;
+    lobj->attrib_ref();
+  }
+}
+
+/**
+ * Destructor.
+ */
+LightAttrib::
+~LightAttrib() {
+  // Call attrib_unref() on all on lights.
+  Lights::const_iterator it;
+  for (it = _on_lights.begin(); it != _on_lights.end(); ++it) {
+    const NodePath &np = *it;
+    if (!np.is_empty()) {
+      Light *lobj = np.node()->as_light();
+      if (lobj != nullptr) {
+        lobj->attrib_unref();
+      }
+    }
+  }
+}
+
 /**
  * Constructs a new LightAttrib object that turns on (or off, according to op)
  * the indicated light(s).
@@ -376,14 +414,17 @@ make_all_off() {
  */
 CPT(RenderAttrib) LightAttrib::
 add_on_light(const NodePath &light) const {
-  nassertr(!light.is_empty() && light.node()->as_light() != (Light *)NULL, this);
+  nassertr(!light.is_empty(), this);
+  Light *lobj = light.node()->as_light();
+  nassertr(lobj != nullptr, this);
+
   LightAttrib *attrib = new LightAttrib(*this);
-  attrib->_on_lights.insert(light);
-  attrib->_off_lights.erase(light);
 
   pair<Lights::iterator, bool> insert_result =
     attrib->_on_lights.insert(Lights::value_type(light));
   if (insert_result.second) {
+    lobj->attrib_ref();
+
     // Also ensure it is removed from the off_lights list.
     attrib->_off_lights.erase(light);
   }
@@ -397,9 +438,14 @@ add_on_light(const NodePath &light) const {
  */
 CPT(RenderAttrib) LightAttrib::
 remove_on_light(const NodePath &light) const {
-  nassertr(!light.is_empty() && light.node()->as_light() != (Light *)NULL, this);
+  nassertr(!light.is_empty(), this);
+  Light *lobj = light.node()->as_light();
+  nassertr(lobj != nullptr, this);
+
   LightAttrib *attrib = new LightAttrib(*this);
-  attrib->_on_lights.erase(light);
+  if (attrib->_on_lights.erase(light)) {
+    lobj->attrib_unref();
+  }
   return return_new(attrib);
 }
 
@@ -409,12 +455,17 @@ remove_on_light(const NodePath &light) const {
  */
 CPT(RenderAttrib) LightAttrib::
 add_off_light(const NodePath &light) const {
-  nassertr(!light.is_empty() && light.node()->as_light() != (Light *)NULL, this);
+  nassertr(!light.is_empty(), this);
+  Light *lobj = light.node()->as_light();
+  nassertr(lobj != nullptr, this);
+
   LightAttrib *attrib = new LightAttrib(*this);
   if (!_off_all_lights) {
     attrib->_off_lights.insert(light);
   }
-  attrib->_on_lights.erase(light);
+  if (attrib->_on_lights.erase(light)) {
+    lobj->attrib_unref();
+  }
   return return_new(attrib);
 }
 
@@ -960,6 +1011,10 @@ finalize(BamReader *manager) {
         // If it's in the registry, replace it.
         _on_lights[i] = areg->get_node(n);
       }
+
+      Light *lobj = _on_lights[i].node()->as_light();
+      nassertd(lobj != nullptr) continue;
+      lobj->attrib_ref();
     }
 
   } else {
@@ -992,10 +1047,15 @@ finalize(BamReader *manager) {
       if (n != -1) {
         // If it's in the registry, add that NodePath.
         _on_lights.push_back(areg->get_node(n));
+        node = _on_lights.back().node();
       } else {
         // Otherwise, add any arbitrary NodePath.  Complain if it's ambiguous.
         _on_lights.push_back(NodePath(node));
       }
+
+      Light *lobj = node->as_light();
+      nassertd(lobj != nullptr) continue;
+      lobj->attrib_ref();
     }
   }
 
diff --git a/panda/src/pgraph/lightAttrib.h b/panda/src/pgraph/lightAttrib.h
@@ -30,9 +30,10 @@
 class EXPCL_PANDA_PGRAPH LightAttrib : public RenderAttrib {
 protected:
   INLINE LightAttrib();
-  INLINE LightAttrib(const LightAttrib &copy);
+  LightAttrib(const LightAttrib &copy);
 
 PUBLISHED:
+  virtual ~LightAttrib();
 
   // This is the old, deprecated interface to LightAttrib.  Do not use any of
   // these methods for new code; these methods will be removed soon.
diff --git a/panda/src/pgraphnodes/lightLensNode.cxx b/panda/src/pgraphnodes/lightLensNode.cxx
@@ -27,7 +27,8 @@ TypeHandle LightLensNode::_type_handle;
  */
 LightLensNode::
 LightLensNode(const string &name, Lens *lens) :
-  Camera(name, lens)
+  Camera(name, lens),
+  _attrib_count(0)
 {
   set_active(false);
   _shadow_caster = false;
@@ -46,6 +47,10 @@ LightLensNode::
 ~LightLensNode() {
   set_active(false);
   clear_shadow_buffers();
+
+  // If this triggers, the number of attrib_ref() didn't match the number of
+  // attrib_unref() calls, probably indicating a bug in LightAttrib.
+  nassertv(AtomicAdjust::get(_attrib_count) == 0);
 }
 
 /**
@@ -57,7 +62,8 @@ LightLensNode(const LightLensNode &copy) :
   Camera(copy),
   _shadow_caster(copy._shadow_caster),
   _sb_size(copy._sb_size),
-  _sb_sort(-10)
+  _sb_sort(-10),
+  _attrib_count(0)
 {
 }
 
@@ -81,6 +87,28 @@ clear_shadow_buffers() {
   _sbuffers.clear();
 }
 
+
+/**
+ * This is called when the light is added to a LightAttrib.
+ */
+void LightLensNode::
+attrib_ref() {
+  AtomicAdjust::inc(_attrib_count);
+}
+
+/**
+ * This is called when the light is removed from a LightAttrib.
+ */
+void LightLensNode::
+attrib_unref() {
+  // When it is removed from the last LightAttrib, destroy the shadow buffers.
+  // This is necessary to break the circular reference that the buffer holds
+  // on this node, via the display region's camera.
+  if (!AtomicAdjust::dec(_attrib_count)) {
+    clear_shadow_buffers();
+  }
+}
+
 /**
  * Returns the Light object upcast to a PandaNode.
  */
diff --git a/panda/src/pgraphnodes/lightLensNode.h b/panda/src/pgraphnodes/lightLensNode.h
@@ -20,6 +20,7 @@
 #include "camera.h"
 #include "graphicsStateGuardianBase.h"
 #include "graphicsOutputBase.h"
+#include "atomicAdjust.h"
 
 class ShaderGenerator;
 class GraphicsStateGuardian;
@@ -59,7 +60,14 @@ class EXPCL_PANDA_PGRAPHNODES LightLensNode : public Light, public Camera {
   typedef pmap<PT(GraphicsStateGuardianBase), PT(GraphicsOutputBase) > ShadowBuffers;
   ShadowBuffers _sbuffers;
 
+  // This counts how many LightAttribs in the world are referencing this
+  // LightLensNode object.
+  AtomicAdjust::Integer _attrib_count;
+
 public:
+  virtual void attrib_ref();
+  virtual void attrib_unref();
+
   virtual PandaNode *as_node();
   virtual Light *as_light();
 
