From 9a40febdb9ce40a0936d164a2ed3d72ddf7dcc8a Mon Sep 17 00:00:00 2001 From: rdb Date: Sun, 9 Jul 2017 20:33:45 +0200 Subject: [PATCH] shadows: fix light buffer not being destructed after light removal Fixes LP 1672089 --- panda/src/pgraph/light.cxx | 14 +++++ panda/src/pgraph/light.h | 3 + panda/src/pgraph/lightAttrib.I | 13 ----- panda/src/pgraph/lightAttrib.cxx | 74 ++++++++++++++++++++++--- panda/src/pgraph/lightAttrib.h | 3 +- panda/src/pgraphnodes/lightLensNode.cxx | 32 ++++++++++- panda/src/pgraphnodes/lightLensNode.h | 8 +++ 7 files changed, 124 insertions(+), 23 deletions(-) diff --git a/panda/src/pgraph/light.cxx b/panda/src/pgraph/light.cxx index b159450142..82b53f5757 100644 --- 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 index eeca3409bd..ee6e906356 100644 --- a/panda/src/pgraph/light.h +++ b/panda/src/pgraph/light.h @@ -64,6 +64,9 @@ PUBLISHED: 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 index e876055642..0c6686181b 100644 --- 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 ©) : - _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 index 8bb66bcec2..2fb58c106d 100644 --- a/panda/src/pgraph/lightAttrib.cxx +++ b/panda/src/pgraph/lightAttrib.cxx @@ -47,6 +47,44 @@ public: } }; +/** + * 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 ©) : + _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 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 index 1158531bc0..eaa86ff895 100644 --- 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 ©); + LightAttrib(const LightAttrib ©); 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 index bc3e134116..a843123cff 100644 --- 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 ©) : 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 index 0c4b329b16..11c259e5e5 100644 --- 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 @@ protected: typedef pmap 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();