Fixed incorrect weak pointer manipulation (Drawable's cached Zone) from worker threads, which caused a memory leak.

Tidied up SharedPtr/WeakPtr code.

Engine/Container/ArrayPtr.h

@@ -46,11 +46,7 @@ public:
         ptr_(rhs.ptr_),
         refCount_(rhs.refCount_)
     {
-        if (refCount_)
-        {
-            assert(refCount_->refs_ > 0);
-            ++(refCount_->refs_);
-        }
+        AddRef();
     }
     
     /// Construct from a raw pointer.
@@ -58,13 +54,13 @@ public:
         ptr_(ptr),
         refCount_(new RefCount())
     {
-        ++(refCount_->refs_);
+        AddRef();
     }
     
     /// Destruct. Release the array reference.
     ~SharedArrayPtr()
     {
-        Release();
+        ReleaseRef();
     }
     
     /// Assign from another shared array pointer.
@@ -73,15 +69,10 @@ public:
         if (ptr_ == rhs.ptr_)
             return *this;
         
-        Release();
-        
+        ReleaseRef();
         ptr_ = rhs.ptr_;
         refCount_ = rhs.refCount_;
-        if (refCount_)
-        {
-            assert(refCount_->refs_ > 0);
-            ++(refCount_->refs_);
-        }
+        AddRef();
         
         return *this;
     }
@@ -92,13 +83,13 @@ public:
         if (ptr_ == ptr)
             return *this;
         
-        Release();
+        ReleaseRef();
         
         if (ptr)
         {
             ptr_ = ptr;
             refCount_ = new RefCount();
-            ++(refCount_->refs_);
+            AddRef();
         }
         
         return *this;
@@ -122,30 +113,27 @@ public:
     operator T* () const { return ptr_; }
     
     /// Reset to null and release the array reference.
-    void Reset() { Release(); }
+    void Reset() { ReleaseRef(); }
     
     /// Perform a static cast from a shared array pointer of another type.
     template <class U> void StaticCast(const SharedArrayPtr<U>& rhs)
     {
-        Release();
-        
+        ReleaseRef();
         ptr_ = static_cast<T*>(rhs.Get());
         refCount_ = rhs.RefCountPtr();
-        if (refCount_)
-            ++(refCount_->refs_);
+        AddRef();
     }
     
     /// Perform a dynamic cast from a shared array pointer of another type.
     template <class U> void DynamicCast(const SharedArrayPtr<U>& rhs)
     {
-        Release();
-        
+        ReleaseRef();
         ptr_ = dynamic_cast<T*>(rhs.Get());
+        
         if (ptr_)
         {
             refCount_ = rhs.RefCountPtr();
-            assert(refCount_->refs_ > 0);
-            ++(refCount_->refs_);
+            AddRef();
         }
         else
             refCount_ = 0;
@@ -170,8 +158,18 @@ private:
     /// Prevent direct assignment from a shared array pointer of different type.
     template <class U> SharedArrayPtr<T>& operator = (const SharedArrayPtr<U>& rhs);
     
+    /// Add a reference to the array pointed to.
+    void AddRef()
+    {
+        if (refCount_)
+        {
+            assert(refCount_->refs_ >= 0);
+            ++(refCount_->refs_);
+        }
+    }
+    
     /// Release the array reference and delete it and the RefCount structure if necessary.
-    void Release()
+    void ReleaseRef()
     {
         if (refCount_)
         {
@@ -224,34 +222,26 @@ public:
     {
     }
     
-    /// Construct from a shared array pointer.
-    WeakArrayPtr(const SharedArrayPtr<T>& rhs) :
-        ptr_(rhs.Get()),
-        refCount_(rhs.RefCountPtr())
-    {
-        if (refCount_)
-        {
-            assert(refCount_->weakRefs_ >= 0);
-            ++(refCount_->weakRefs_);
-        }
-    }
-    
     /// Copy-construct from another weak array pointer.
     WeakArrayPtr(const WeakArrayPtr<T>& rhs) :
         ptr_(rhs.ptr_),
         refCount_(rhs.refCount_)
     {
-        if (refCount_)
-        {
-            assert(refCount_->weakRefs_ >= 0);
-            ++(refCount_->weakRefs_);
-        }
+        AddRef();
+    }
+    
+    /// Construct from a shared array pointer.
+    WeakArrayPtr(const SharedArrayPtr<T>& rhs) :
+        ptr_(rhs.Get()),
+        refCount_(rhs.RefCountPtr())
+    {
+        AddRef();
     }
     
     /// Destruct. Release the weak reference to the array.
     ~WeakArrayPtr()
     {
-        Release();
+        ReleaseRef();
     }
     
     /// Assign from a shared array pointer.
@@ -260,15 +250,11 @@ public:
         if (ptr_ == rhs.Get() && refCount_ == rhs.RefCountPtr())
             return *this;
         
-        Release();
-        
+        ReleaseRef();
         ptr_ = rhs.Get();
         refCount_ = rhs.RefCountPtr();
-        if (refCount_)
-        {
-            assert(refCount_->weakRefs_ >= 0);
-            ++(refCount_->weakRefs_);
-        }
+        AddRef();
+        
         return *this;
     }
     
@@ -278,15 +264,10 @@ public:
         if (ptr_ == rhs.ptr_ && refCount_ == rhs.refCount_)
             return *this;
         
-        Release();
-        
+        ReleaseRef();
         ptr_ = rhs.ptr_;
         refCount_ = rhs.refCount_;
-        if (refCount_)
-        {
-            assert(refCount_->weakRefs_ >= 0);
-            ++(refCount_->weakRefs_);
-        }
+        AddRef();
         
         return *this;
     }
@@ -350,31 +331,22 @@ public:
     /// Perform a static cast from a weak array pointer of another type.
     template <class U> void StaticCast(const WeakArrayPtr<U>& rhs)
     {
-        Release();
-        
+        ReleaseRef();
         ptr_ = static_cast<T*>(rhs.Get());
         refCount_ = rhs.refCount_;
-        if (refCount_)
-        {
-            assert(refCount_->weakRefs_ >= 0);
-            ++(refCount_->weakRefs_);
-        }
+        AddRef();
     }
     
     /// Perform a dynamic cast from a weak array pointer of another type.
     template <class U> void DynamicCast(const WeakArrayPtr<U>& rhs)
     {
-        Release();
-        
+        ReleaseRef();
         ptr_ = dynamic_cast<T*>(rhs.Get());
+        
         if (ptr_)
         {
             refCount_ = rhs.refCount_;
-            if (refCount_)
-            {
-                assert(refCount_->weakRefs_ >= 0);
-                ++(refCount_->weakRefs_);
-            }
+            AddRef();
         }
         else
             refCount_ = 0;
@@ -399,8 +371,18 @@ private:
     /// Prevent direct assignment from a weak array pointer of different type.
     template <class U> WeakArrayPtr<T>& operator = (const WeakArrayPtr<U>& rhs);
     
+    /// Add a weak reference to the array pointed to.
+    void AddRef()
+    {
+        if (refCount_)
+        {
+            assert(refCount_->weakRefs_ >= 0);
+            ++(refCount_->weakRefs_);
+        }
+    }
+    
     /// Release the weak reference. Delete the Refcount structure if necessary.
-    void Release()
+    void ReleaseRef()
     {
         if (refCount_)
         {

Engine/Container/Ptr.h

@@ -43,22 +43,20 @@ public:
     SharedPtr(const SharedPtr<T>& rhs) :
         ptr_(rhs.ptr_)
     {
-        if (ptr_)
-            ptr_->AddRef();
+        AddRef();
     }
     
     /// Construct from a raw pointer.
     explicit SharedPtr(T* ptr) :
         ptr_(ptr)
     {
-        if (ptr_)
-            ptr_->AddRef();
+        AddRef();
     }
     
     /// Destruct. Release the object reference.
     ~SharedPtr()
     {
-        Release();
+        ReleaseRef();
     }
     
     /// Assign from another shared pointer.
@@ -67,11 +65,9 @@ public:
         if (ptr_ == rhs.ptr_)
             return *this;
         
-        Release();
-        
+        ReleaseRef();
         ptr_ = rhs.ptr_;
-        if (ptr_)
-            ptr_->AddRef();
+        AddRef();
         
         return *this;
     }
@@ -82,11 +78,9 @@ public:
         if (ptr_ == ptr)
             return *this;
         
-        Release();
-        
+        ReleaseRef();
         ptr_ = ptr;
-        if (ptr_)
-            ptr_->AddRef();
+        AddRef();
         
         return *this;
     }
@@ -109,26 +103,22 @@ public:
     operator T* () const { return ptr_; }
     
     /// Reset to null and release the object reference.
-    void Reset() { Release(); }
+    void Reset() { ReleaseRef(); }
     
     /// Perform a static cast from a shared pointer of another type.
     template <class U> void StaticCast(const SharedPtr<U>& rhs)
     {
-        Release();
-        
+        ReleaseRef();
         ptr_ = static_cast<T*>(rhs.Get());
-        if (ptr_)
-            ptr_->AddRef();
+        AddRef();
     }
     
     /// Perform a dynamic cast from a shared pointer of another type.
     template <class U> void DynamicCast(const SharedPtr<U>& rhs)
     {
-        Release();
-        
+        ReleaseRef();
         ptr_ = dynamic_cast<T*>(rhs.Get());
-        if (ptr_)
-            ptr_->AddRef();
+        AddRef();
     }
     
     /// Check if the pointer is null.
@@ -150,8 +140,15 @@ private:
     /// Prevent direct assignment from a shared pointer of another type.
     template <class U> SharedPtr<T>& operator = (const SharedPtr<U>& rhs);
     
+    /// Add a reference to the object pointed to.
+    void AddRef()
+    {
+        if (ptr_)
+            ptr_->AddRef();
+    }
+    
     /// Release the object reference and delete it if necessary.
-    void Release()
+    void ReleaseRef()
     {
         if (ptr_)
         {
@@ -191,28 +188,20 @@ public:
     {
     }
     
-    /// Construct from a shared pointer.
-    WeakPtr(const SharedPtr<T>& rhs) :
-        ptr_(rhs.Get()),
-        refCount_(rhs.RefCountPtr())
-    {
-        if (refCount_)
-        {
-            assert(refCount_->weakRefs_ >= 0);
-            ++(refCount_->weakRefs_);
-        }
-    }
-    
     /// Copy-construct from another weak pointer.
     WeakPtr(const WeakPtr<T>& rhs) :
         ptr_(rhs.ptr_),
         refCount_(rhs.refCount_)
     {
-        if (refCount_)
-        {
-            assert(refCount_->weakRefs_ >= 0);
-            ++(refCount_->weakRefs_);
-        }
+        AddRef();
+    }
+    
+    /// Construct from a shared pointer.
+    WeakPtr(const SharedPtr<T>& rhs) :
+        ptr_(rhs.Get()),
+        refCount_(rhs.RefCountPtr())
+    {
+        AddRef();
     }
     
     /// Construct from a raw pointer.
@@ -220,17 +209,13 @@ public:
         ptr_(ptr),
         refCount_(ptr ? ptr->RefCountPtr() : 0)
     {
-        if (refCount_)
-        {
-            assert(refCount_->weakRefs_ >= 0);
-            ++(refCount_->weakRefs_);
-        }
+        AddRef();
     }
     
     /// Destruct. Release the weak reference to the object.
     ~WeakPtr()
     {
-        Release();
+        ReleaseRef();
     }
     
     /// Assign from a shared pointer.
@@ -239,15 +224,10 @@ public:
         if (ptr_ == rhs.Get() && refCount_ == rhs.RefCountPtr())
             return *this;
         
-        Release();
-        
+        ReleaseRef();
         ptr_ = rhs.Get();
         refCount_ = rhs.RefCountPtr();
-        if (refCount_)
-        {
-            assert(refCount_->weakRefs_ >= 0);
-            ++(refCount_->weakRefs_);
-        }
+        AddRef();
         
         return *this;
     }
@@ -258,15 +238,10 @@ public:
         if (ptr_ == rhs.ptr_ && refCount_ == rhs.refCount_)
             return *this;
         
-        Release();
-        
+        ReleaseRef();
         ptr_ = rhs.ptr_;
         refCount_ = rhs.refCount_;
-        if (refCount_)
-        {
-            assert(refCount_->weakRefs_ >= 0);
-            ++(refCount_->weakRefs_);
-        }
+        AddRef();
         
         return *this;
     }
@@ -279,15 +254,10 @@ public:
         if (ptr_ == ptr && refCount_ == refCount)
             return *this;
         
-        Release();
-        
+        ReleaseRef();
         ptr_ = ptr;
         refCount_ = refCount;
-        if (refCount_)
-        {
-            assert(refCount_->weakRefs_ >= 0);
-            ++(refCount_->weakRefs_);
-        }
+        AddRef();
         
         return *this;
     }
@@ -346,36 +316,27 @@ public:
     operator T* () const { return Get(); }
     
     /// Reset to null and release the weak reference.
-    void Reset() { Release(); }
+    void Reset() { ReleaseRef(); }
     
     /// Perform a static cast from a weak pointer of another type.
     template <class U> void StaticCast(const WeakPtr<U>& rhs)
     {
-        Release();
-        
+        ReleaseRef();
         ptr_ = static_cast<T*>(rhs.Get());
         refCount_ = rhs.refCount_;
-        if (refCount_)
-        {
-            assert(refCount_->weakRefs_ >= 0);
-            ++(refCount_->weakRefs_);
-        }
+        AddRef();
     }
     
     /// Perform a dynamic cast from a weak pointer of another type.
     template <class U> void DynamicCast(const WeakPtr<U>& rhs)
     {
-        Release();
-        
+        ReleaseRef();
         ptr_ = dynamic_cast<T*>(rhs.Get());
+        
         if (ptr_)
         {
             refCount_ = rhs.refCount_;
-            if (refCount_)
-            {
-                assert(refCount_->weakRefs_ >= 0);
-                ++(refCount_->weakRefs_);
-            }
+            AddRef();
         }
         else
             refCount_ = 0;
@@ -408,8 +369,18 @@ private:
     /// Prevent direct assignment from a weak pointer of different type.
     template <class U> WeakPtr<T>& operator = (const WeakPtr<U>& rhs);
     
+    /// Add a weak reference to the object pointed to.
+    void AddRef()
+    {
+        if (refCount_)
+        {
+            assert(refCount_->weakRefs_ >= 0);
+            ++(refCount_->weakRefs_);
+        }
+    }
+    
     /// Release the weak reference. Delete the Refcount structure if necessary.
-    void Release()
+    void ReleaseRef()
     {
         if (refCount_)
         {

Engine/Container/RefCounted.cpp

@@ -33,7 +33,7 @@ RefCounted::RefCounted() :
     refCount_(new RefCount())
 {
     // Hold a weak ref to self to avoid possible double delete of the refcount
-    ++(refCount_->weakRefs_);
+    (refCount_->weakRefs_)++;
 }
 
 RefCounted::~RefCounted()
@@ -44,7 +44,7 @@ RefCounted::~RefCounted()
     
     // Mark object as expired, release the self weak ref and delete the refcount if no other weak refs exist
     refCount_->refs_ = -1;
-    --(refCount_->weakRefs_);
+    (refCount_->weakRefs_)--;
     if (!refCount_->weakRefs_)
         delete refCount_;
     
@@ -54,13 +54,13 @@ RefCounted::~RefCounted()
 void RefCounted::AddRef()
 {
     assert(refCount_->refs_ >= 0);
-    ++(refCount_->refs_);
+    (refCount_->refs_)++;
 }
 
 void RefCounted::ReleaseRef()
 {
     assert(refCount_->refs_ > 0);
-    --(refCount_->refs_);
+    (refCount_->refs_)--;
     if (!refCount_->refs_)
         delete this;
 }

Engine/Graphics/Drawable.cpp

@@ -82,7 +82,7 @@ Drawable::Drawable(Context* context, unsigned char drawableFlags) :
     firstLight_(0),
     viewFrame_(0),
     viewCamera_(0),
-    temporaryZone_(false)
+    zoneDirty_(false)
 {
 }
 
@@ -248,7 +248,9 @@ void Drawable::SetZone(Zone* zone, bool temporary)
 {
     zone_ = zone;
     lastZone_ = zone;
-    temporaryZone_ = temporary;
+    
+    // If the zone assignment was temporary (inconclusive) set the dirty flag so that it will be re-evaluated on the next frame
+    zoneDirty_ = temporary;
 }
 
 void Drawable::SetSortValue(float value)
@@ -266,13 +268,6 @@ void Drawable::MarkInView(const FrameInfo& frame, bool mainView)
 {
     if (mainView)
     {
-        // Reset zone assignment now if was temporary
-        if (temporaryZone_)
-        {
-            zone_.Reset();
-            temporaryZone_ = false;
-        }
-        
         viewFrameNumber_ = frame.frameNumber_;
         viewFrame_ = &frame;
         viewCamera_ = frame.camera_;
@@ -364,8 +359,10 @@ void Drawable::OnMarkedDirty(Node* node)
     if (!reinsertionQueued_ && octant_)
         octant_->GetRoot()->QueueReinsertion(this);
     
+    // Mark zone assignment dirty. Due to possibly being called from a worker thread, it is unsafe to manipulate the Zone weak
+    // pointer here
     if (node == node_)
-        zone_.Reset();
+        zoneDirty_ = true;
 }
 
 void Drawable::AddToOctree()

Engine/Graphics/Drawable.h

@@ -219,6 +219,8 @@ public:
     Zone* GetZone() const;
     /// Return previous zone.
     Zone* GetLastZone() const;
+    /// Return if zone assignment needs re-evaluation.
+    bool IsZoneDirty() const { return zoneDirty_; }
     /// Return distance from camera.
     float GetDistance() const { return distance_; }
     /// Return LOD scaled distance from camera.
@@ -322,8 +324,8 @@ protected:
     const FrameInfo* viewFrame_;
     /// Last view's camera. Not safe to dereference.
     Camera* viewCamera_;
-    /// Zone assignment temporary flag.
-    bool temporaryZone_;
+    /// Zone assignment dirty flag.
+    bool zoneDirty_;
 };
 
 inline bool CompareDrawables(Drawable* lhs, Drawable* rhs)

Engine/Graphics/View.cpp

@@ -646,7 +646,7 @@ void View::GetDrawables()
         if (drawable->GetDrawableFlags() & DRAWABLE_GEOMETRY)
         {
             // Find zone for the drawable if necessary
-            if (!drawable->GetZone() && !cameraZoneOverride_)
+            if ((drawable->IsZoneDirty() || !drawable->GetZone()) && !cameraZoneOverride_)
                 FindZone(drawable);
             
             // Expand the scene bounding box and Z range (skybox not included because of infinite size) and store the drawawble

Engine/Graphics/Zone.cpp

@@ -192,7 +192,7 @@ bool Zone::IsInside(const Vector3& point) const
 
 void Zone::OnMarkedDirty(Node* node)
 {
-    // Due to the octree query, is not safe from worker threads
+    // Due to the octree query and weak pointer manipulation, is not safe from worker threads
     Scene* scene = GetScene();
     if (scene && scene->IsThreadedUpdate())
     {

Engine/Graphics/Zone.h

@@ -69,9 +69,9 @@ public:
     const Matrix3x4& GetInverseWorldTransform() const;
     /// Return zone's own ambient color, disregarding gradient mode.
     const Color& GetAmbientColor() const { return ambientColor_; }
-    /// Return ambient start color.
+    /// Return ambient start color. Not safe to call from worker threads due to possible octree query.
     const Color& GetAmbientStartColor();
-    /// Return ambient end color.
+    /// Return ambient end color. Not safe to call from worker threads due to possible octree query.
     const Color& GetAmbientEndColor();
     /// Return fog color.
     const Color& GetFogColor() const { return fogColor_; }
@@ -94,7 +94,7 @@ protected:
     virtual void OnMarkedDirty(Node* node);
     /// Recalculate the world-space bounding box.
     virtual void OnWorldBoundingBoxUpdate();
-    /// Recalculate the ambient gradient colors from neighbor zones.
+    /// Recalculate the ambient gradient colors from neighbor zones. Not safe to call from worker threads due to octree query.
     void UpdateAmbientGradient();
     
     /// Cached inverse world transform matrix.