Change picking manager to not own objects associated with pickers

When adding a vtkPicker to the picking manager, a vtkObject can optionally be
associated with the picker. This commit changes the behavior of
vtkPickingManager to not own (i.e. increment the reference count) of these
associated objects. Instead, the object references are held in a raw pointer.

The widget framework is the primary client of the picking manager. The typical
vtkWidgetRepresentation subclass adds its pickers to the picking manager and
includes an associated object reference to itself:

    pickingManager->AddPicker(picker, this);

When the widget/representation are destroyed, the vtkWidgetRepresentation
destructor unregisters its pickers:

    pickingManager->RemoveObject(this);

However, because the picking manager took ownership of the associated objects,
the vtkWidgetRepresentation instances and their pickers are not destroyed until
the vtkPickingManager is destroyed. In particular applications, this could lead
to a large number of objects referenced only by vtkPickingManager that would
exist until application exit.

The picking manager test is updated for the use case demonstrated by the widget
framework.

Author: msmolens

Rendering/Core/Testing/Cxx/TestPickingManager.cxx

@@ -53,6 +53,7 @@ class PickingManagerTest
   bool TestAddPickers();
   bool TestRemovePickers();
   bool TestRemoveObjects();
+  bool TestObjectOwnership();
 
   bool VTKVerify(bool test, const char* errorStr, int line);
   void PrintErrorMessage(int line, const char* errorStr);
@@ -87,6 +88,73 @@ class PickingManagerTest
   vtkSmartPointer<vtkPickingManager> PickingManager;
 };
 
+//------------------------------------------------------------------------------
+// Test picking manager client that removes itself from the picking manager
+// in its destructor. This mimics the behavior of the VTK widget framework.
+class PickingManagerClient : public vtkObject
+  {
+  public:
+    static PickingManagerClient* New();
+    vtkTypeMacro(PickingManagerClient, vtkObject);
+
+    void SetPickingManager(vtkPickingManager *pm);
+    void RegisterPicker();
+    vtkPicker* GetPicker();
+
+  protected:
+    PickingManagerClient();
+    ~PickingManagerClient();
+
+  private:
+    vtkPickingManager *PickingManager;
+    vtkPicker *Picker;
+
+    PickingManagerClient(const PickingManagerClient&);  //Not implemented
+    void operator=(const PickingManagerClient&);  //Not implemented
+  };
+
+vtkStandardNewMacro(PickingManagerClient);
+
+//------------------------------------------------------------------------------
+PickingManagerClient::PickingManagerClient()
+{
+  this->Picker = vtkPicker::New();
+}
+
+//------------------------------------------------------------------------------
+PickingManagerClient::~PickingManagerClient()
+{
+  this->Picker->Delete();
+
+  if (this->PickingManager)
+    {
+    this->PickingManager->RemoveObject(this);
+    }
+}
+
+//------------------------------------------------------------------------------
+void PickingManagerClient::SetPickingManager(vtkPickingManager *pm)
+{
+  this->PickingManager = pm;
+}
+
+//------------------------------------------------------------------------------
+void PickingManagerClient::RegisterPicker()
+{
+  if (!this->PickingManager)
+    {
+    return;
+    }
+
+  this->PickingManager->AddPicker(this->Picker, this);
+}
+
+//------------------------------------------------------------------------------
+vtkPicker* PickingManagerClient::GetPicker()
+{
+  return this->Picker;
+}
+
 //------------------------------------------------------------------------------
 int TestPickingManager(int, char*[])
 {
@@ -98,6 +166,7 @@ int TestPickingManager(int, char*[])
   res = res && pickingManagerTest.TestAddPickers();
   res = res && pickingManagerTest.TestRemovePickers();
   res = res && pickingManagerTest.TestRemoveObjects();
+  res = res && pickingManagerTest.TestObjectOwnership();
 
   return res ? EXIT_SUCCESS : EXIT_FAILURE;
 }
@@ -253,6 +322,28 @@ bool PickingManagerTest::TestRemoveObjects()
   return res;
 }
 
+//------------------------------------------------------------------------------
+bool PickingManagerTest::TestObjectOwnership()
+{
+  bool res = true;
+
+  this->PickingManager = vtkSmartPointer<vtkPickingManager>::New();
+  vtkSmartPointer<PickingManagerClient> client =
+    vtkSmartPointer<PickingManagerClient>::New();
+  client->SetPickingManager(this->PickingManager.GetPointer());
+  client->RegisterPicker();
+
+  res = VTK_VERIFY(this->CheckState(1, client->GetPicker(), 1),
+                   "Error after client registers picker:") && res;
+
+  client = NULL;
+
+  res = VTK_VERIFY(this->CheckState(0, NULL, 0),
+                   "Error after setting client object to NULL:") && res;
+
+  return res;
+}
+
 //------------------------------------------------------------------------------
 std::pair<vtkSmartPointer<vtkPicker>, vtkSmartPointer<vtkObject> > PickingManagerTest::
 AddPickerObject(int pickerType, int objectType)

Rendering/Core/vtkPickingManager.cxx

@@ -88,14 +88,14 @@ class vtkPickingManager::vtkInternal
   // Create a new list of associated observers
   void CreateDefaultCollection(vtkAbstractPicker* picker, vtkObject* object);
 
-  // Instead of a vtkCollection we are using a vector of a vtkSmartPointer
+  // vtkCollection doesn't allow NULL values. Instead we use a vector
   // containing vtkObject to allow using 0 as a valid value because it is
   // allowed the return a picker event if he is not associated to a specific
   // object.
   // This is related with the capacity when a picker associated with a given
   // object does not manage others object,
   // it will automatically be removed from the list as well.
-  typedef std::vector<vtkSmartPointer<vtkObject> > CollectionType;
+  typedef std::vector<vtkObject*> CollectionType;
 
   // For code clearance and performances during the computation std::map is
   // used instead of a vector of pair. Nevertheless, it makes internally use of
@@ -138,23 +138,6 @@ class vtkPickingManager::vtkInternal
     vtkAbstractPicker* Picker;
   };
 
-  // Predicate comparing a vtkObject*
-  // and a vtkSmartPointer<vtkObject> using the PickerObjectsType.
-  // As we use a vtkSmartPointer, this predicate allows to compare the equality
-  // of a pointer on a vtkObject with the adress contained in
-  // a corresponding vtkSmartPointer.
-  struct equal_smartPtrObject
-  {
-    equal_smartPtrObject(vtkObject* object) : Object(object) {}
-
-    bool operator () (const vtkSmartPointer<vtkObject>& smartObj) const
-      {
-      return this->Object == smartObj.GetPointer();
-      }
-
-    vtkObject* Object;
-  };
-
   PickerObjectsType Pickers;           // Map the picker with the objects
   vtkTimeStamp CurrentInteractionTime; // Time of the last interaction event
   vtkTimeStamp LastPickingTime;        // Time of the last picking process
@@ -202,9 +185,9 @@ CreateDefaultCollection(vtkAbstractPicker* picker, vtkObject* object)
 void vtkPickingManager::vtkInternal::
 LinkPickerObject(const PickerObjectsType::iterator& it, vtkObject* object)
 {
-  CollectionType::iterator itObj = std::find_if(it->second.begin(),
-                                                it->second.end(),
-                                                equal_smartPtrObject(object));
+  CollectionType::iterator itObj = std::find(it->second.begin(),
+                                             it->second.end(),
+                                             object);
 
   if (itObj != it->second.end() && object)
     {
@@ -234,9 +217,9 @@ IsObjectLinked(vtkAbstractPicker* picker, vtkObject* obj)
     return false;
     }
 
-  CollectionType::iterator itObj = std::find_if(itPick->second.begin(),
-                                                itPick->second.end(),
-                                                equal_smartPtrObject(obj));
+  CollectionType::iterator itObj = std::find(itPick->second.begin(),
+                                             itPick->second.end(),
+                                             obj);
   return (itObj != itPick->second.end());
 }
 
@@ -418,9 +401,9 @@ void vtkPickingManager::RemovePicker(vtkAbstractPicker* picker,
     }
 
   vtkPickingManager::vtkInternal::CollectionType::iterator itObj =
-    std::find_if(it->second.begin(),
-                 it->second.end(),
-                 vtkPickingManager::vtkInternal::equal_smartPtrObject(object));
+    std::find(it->second.begin(),
+              it->second.end(),
+              object);
 
   // The object is not associated with the given picker.
   if (itObj == it->second.end())
@@ -446,9 +429,9 @@ void vtkPickingManager::RemoveObject(vtkObject* object)
   for(; it != this->Internal->Pickers.end();)
     {
     vtkPickingManager::vtkInternal::CollectionType::iterator itObj =
-      std::find_if(it->second.begin(),
-                   it->second.end(),
-                   vtkPickingManager::vtkInternal::equal_smartPtrObject(object));
+      std::find(it->second.begin(),
+                it->second.end(),
+                object);
 
     if (itObj != it->second.end())
       {