BUG: Fixed ctkVTKDataSetArrayComboBox crashing and not updating

1. ctkVTKDataSetArrayComboBox caused application crash when it is used for a VTK data set that is generated by a pipeline and the combobox was used for setting active scalar.

When a data set is changed due to a pipeline change, the data set is first reset and then recomputed. During reset the scalars are removed therefore the selection in the combobox is changed to item (-1), which triggers a data update. During update all the scalars are removed (again, so there is no change) but then the scalars are re-added when the data is recomputed. Qt combobox automatically selects the first item if an item is added, therefore the selection changes to the first active scalar. This change triggers a data set update, which starts with a reset. During reste the scalars are removed, therefore the selection in the combobox is changed to item (-1), which triggers a data update... This is an infinite loop, which causes application crash due to stack overflow.

Solution was to add a NULL item (no active scalar). This is also useful because without a NULL item it is not possible to unselect scalars.
For backward compatibility reasons, by default NULL item is not added (it is not needed if the combobox is not used for setting the active scalar).

2. ctkVTKDataSetArrayComboBox did not contain any arrays that were added after the data set and attribute types are added.

vtkDataSet Modified event is not invoked if arrays are added/removed/renamed, only vtkDataSet.PointData and vtkDataset.CellData object Modified events are invoked. ctkVTKDataSetModel did not observe the point and cell data objects.

Solution was to add observers to the point and cell data objects.

Libs/Visualization/VTK/Widgets/Testing/Cpp/ctkVTKDataSetArrayComboBoxTest1.cpp

@@ -57,6 +57,35 @@ int ctkVTKDataSetArrayComboBoxTest1(int argc, char * argv [] )
   ctkVTKDataSetArrayComboBox comboBox;
   comboBox.dataSetModel()->setAttributeTypes(ctkVTKDataSetModel::AllAttribute);
   comboBox.setDataSet(dataSet.GetPointer());
+
+  if (comboBox.count()!=2)
+    {
+    std::cerr << "Line " << __LINE__ << " - Expected 2 items in the combobox\n"
+                  "\tCurrent count: " << comboBox.count() << "\n";
+    return EXIT_FAILURE;
+    }
+
+  comboBox.setNoneEnabled(true);
+  if (comboBox.count()!=3)
+    {
+    std::cerr << "Line " << __LINE__ << " - Expected 3 items in the combobox\n"
+                  "\tCurrent count: " << comboBox.count() << "\n";
+    return EXIT_FAILURE;
+    }
+  if (!comboBox.itemText(0).isEmpty())
+    {
+    std::cerr << "Line " << __LINE__ << " - First combo box item text is expected to be empty\n";
+    return EXIT_FAILURE;
+    }
+
+  comboBox.setNoneEnabled(false);
+  if (comboBox.count()!=2)
+    {
+    std::cerr << "Line " << __LINE__ << " - Expected 2 items in the combobox\n"
+                  "\tCurrent count: " << comboBox.count() << "\n";
+    return EXIT_FAILURE;
+    }
+
   comboBox.show();
 
   if (argc < 2 || QString(argv[1]) != "-I")

Libs/Visualization/VTK/Widgets/Testing/Cpp/ctkVTKDataSetModelTest1.cpp

@@ -336,6 +336,66 @@ int ctkVTKDataSetModelTest1(int argc, char * argv [] )
     return EXIT_FAILURE;
     }
 
+  dataSetModel.setAttributeTypes(ctkVTKDataSetModel::NoAttribute | ctkVTKDataSetModel::ScalarsAttribute);
+  vtkNew<vtkIntArray> intsLaterAdded;
+  {
+    intsLaterAdded->SetName("IntsLaterAdded");
+    int added = dataSet->GetPointData()->AddArray(intsLaterAdded.GetPointer());
+    locations[intsLaterAdded.GetPointer()] = vtkAssignAttribute::POINT_DATA;
+    if (added == -1)
+      {
+      std::cerr << "Line " << __LINE__ << " - Failed to add intsLaterAdded array";
+      return EXIT_FAILURE;
+      }
+  }
+  if (!checkItems(__LINE__, QList<vtkAbstractArray*>() << notAttributeArrays << intsLaterAdded.GetPointer(),
+                  &dataSetModel, locations))
+    {
+    return EXIT_FAILURE;
+    }
+
+  QList<QStandardItem*> items = dataSetModel.findItems("");
+  if(items.count() != 0)
+    {
+    std::cerr << "Line " << __LINE__ << " - Expected 0 NULL item\n"
+                  "\tCurrent count: " << items.count() << "\n";
+    return EXIT_FAILURE;
+    }
+
+  dataSetModel.setIncludeNullItem(true);
+  items = dataSetModel.findItems("");
+  if(items.count() != 1)
+    {
+    std::cerr << "Line " << __LINE__ << " - Expected 1 NULL item\n"
+                  "\tCurrent count: " << items.count() << "\n";
+    return EXIT_FAILURE;
+    }
+  vtkAbstractArray * currentDataArray = dataSetModel.arrayFromItem(items.at(0));
+  if (currentDataArray != 0)
+    {
+    std::cerr << "Line " << __LINE__ << " - Problem with model - Incorrect array associated with NULL item:\n"
+                  "\tExpected: 0\n"
+                  "\tCurrent: " << currentDataArray << "\n";
+    return EXIT_FAILURE;
+    }
+  int loc = dataSetModel.locationFromItem(items.at(0));
+  if (loc != dataSetModel.nullItemLocation())
+    {
+    std::cerr << "Line " << __LINE__ << " - Problem with model - Incorrect location associated with NULL item:\n"
+                  "\tExpected: " << dataSetModel.nullItemLocation() << "\n"
+                  "\tCurrent: " << loc << "\n";
+    return EXIT_FAILURE;
+    }
+
+  dataSetModel.setIncludeNullItem(false);
+  items = dataSetModel.findItems("");
+  if(items.count() != 0)
+    {
+    std::cerr << "Line " << __LINE__ << " - Expected 0 NULL item\n"
+                  "\tCurrent count: " << items.count() << "\n";
+    return EXIT_FAILURE;
+    }
+
   dataSetModel.setAttributeTypes(ctkVTKDataSetModel::AllAttribute);
   if (!checkItems(__LINE__, QList<vtkAbstractArray*>() << notAttributeArrays
                   << scalars.GetPointer() << vectors.GetPointer()

Libs/Visualization/VTK/Widgets/ctkVTKDataSetArrayComboBox.cpp

@@ -168,6 +168,20 @@ void ctkVTKDataSetArrayComboBox::setAttributeTypes(const ctkVTKDataSetModel::Att
   this->dataSetModel()->setAttributeTypes(attributeTypes);
 }
 
+// ----------------------------------------------------------------------------
+bool ctkVTKDataSetArrayComboBox::noneEnabled()const
+{
+  Q_D(const ctkVTKDataSetArrayComboBox);
+  return this->dataSetModel()->includeNullItem();
+}
+
+// ----------------------------------------------------------------------------
+void ctkVTKDataSetArrayComboBox::setNoneEnabled(bool noneEnabled)
+{
+  Q_D(ctkVTKDataSetArrayComboBox);
+  return this->dataSetModel()->setIncludeNullItem(noneEnabled);
+}
+
 // --------------------------------------------------------------------------
 ctkVTKDataSetModel* ctkVTKDataSetArrayComboBox::dataSetModel()const
 {

Libs/Visualization/VTK/Widgets/ctkVTKDataSetArrayComboBox.h

@@ -40,6 +40,7 @@ class CTK_VISUALIZATION_VTK_WIDGETS_EXPORT ctkVTKDataSetArrayComboBox
 {
   Q_OBJECT
   Q_PROPERTY(ctkVTKDataSetModel::AttributeTypes attributeTypes READ attributeTypes WRITE setAttributeTypes)
+  Q_PROPERTY(bool noneEnabled READ noneEnabled WRITE setNoneEnabled)
 
 public:
   /// Superclass typedef
@@ -61,6 +62,15 @@ class CTK_VISUALIZATION_VTK_WIDGETS_EXPORT ctkVTKDataSetArrayComboBox
   ctkVTKDataSetModel::AttributeTypes attributeTypes()const;
   void setAttributeTypes(const ctkVTKDataSetModel::AttributeTypes& attributeTypes);
 
+  /// Set/Get NoneEnabled flags
+  /// An additional empty item is added into the list, where the user can select.
+  /// It is recommended to enable this if the combobox is used to select active scalar of the
+  /// observed VTK data set, because if there is no None option is available then the combobox selects
+  /// the first array automatically if an array becomes available, causing unintended change of the VTK data set
+  /// (and often infinite loop of widget/MRML node updates).
+  void setNoneEnabled(bool enable);
+  bool noneEnabled()const; 
+
   /// Return a pointer to the model used to populate the combobox.
   /// \sa dataSet()
   ctkVTKDataSetModel* dataSetModel()const;

Libs/Visualization/VTK/Widgets/ctkVTKDataSetModel.cpp

@@ -47,8 +47,12 @@ class ctkVTKDataSetModelPrivate
                                                      vtkDataSetAttributes * dataSetAttributes);
 
   vtkSmartPointer<vtkDataSet> DataSet;
+  vtkSmartPointer<vtkPointData> DataSetPointData;
+  vtkSmartPointer<vtkCellData> DataSetCellData;
+
   bool ListenAbstractArrayModifiedEvent;
   ctkVTKDataSetModel::AttributeTypes AttributeType;
+  bool IncludeNullItem;
 };
 
 
@@ -58,6 +62,7 @@ ctkVTKDataSetModelPrivate::ctkVTKDataSetModelPrivate(ctkVTKDataSetModel& object)
 {
   this->ListenAbstractArrayModifiedEvent = false;
   this->AttributeType = ctkVTKDataSetModel::AllAttribute;
+  this->IncludeNullItem = false;
 }
 
 //------------------------------------------------------------------------------
@@ -138,6 +143,7 @@ QList<vtkAbstractArray*> ctkVTKDataSetModelPrivate::attributeArrayToInsert(
 ctkVTKDataSetModel::ctkVTKDataSetModel(QObject *_parent)
   : QStandardItemModel(_parent)
   , d_ptr(new ctkVTKDataSetModelPrivate(*this))
+  , NullItemLocation(-2) // -1 is already used
 {
   Q_D(ctkVTKDataSetModel);
   d->init();
@@ -168,7 +174,7 @@ void ctkVTKDataSetModel::setDataSet(vtkDataSet* dataSet)
   this->qvtkReconnect(d->DataSet, dataSet, vtkCommand::ModifiedEvent,
                       this, SLOT(onDataSetModified(vtkObject*)) );
   d->DataSet = dataSet;
-  this->updateDataSet();
+  this->onDataSetModified(dataSet);
 }
 
 //------------------------------------------------------------------------------
@@ -197,6 +203,33 @@ void ctkVTKDataSetModel::setAttributeTypes(const AttributeTypes& attributeTypes)
   this->updateDataSet();
 }
 
+// ----------------------------------------------------------------------------
+bool ctkVTKDataSetModel::includeNullItem()const
+{
+  Q_D(const ctkVTKDataSetModel);
+  return d->IncludeNullItem;
+}
+
+// ----------------------------------------------------------------------------
+void ctkVTKDataSetModel::setIncludeNullItem(bool includeNullItem)
+{
+  Q_D(ctkVTKDataSetModel);
+  if (d->IncludeNullItem == includeNullItem)
+    {
+    // no change
+    return;
+    }
+  if (includeNullItem)
+    {
+    this->insertNullItem();
+    }
+  else
+    {
+    this->removeNullItem();
+    }
+  d->IncludeNullItem = includeNullItem;
+}
+
 //------------------------------------------------------------------------------
 vtkAbstractArray* ctkVTKDataSetModel::arrayFromItem(QStandardItem* arrayItem)const
 {
@@ -208,6 +241,13 @@ vtkAbstractArray* ctkVTKDataSetModel::arrayFromItem(QStandardItem* arrayItem)con
   Q_ASSERT(arrayPointer.isValid());
   vtkAbstractArray* array = static_cast<vtkAbstractArray*>(
     reinterpret_cast<void *>(arrayPointer.toLongLong()));
+  if (arrayItem->data(ctkVTK::LocationRole).toInt() == this->NullItemLocation)
+    {
+    // null item
+    Q_ASSERT(array==0);
+    return 0;
+    }
+
   Q_ASSERT(array);
   return array;
 }
@@ -277,7 +317,23 @@ bool ctkVTKDataSetModel::listenNodeModifiedEvent()const
 void ctkVTKDataSetModel::updateDataSet()
 {
   Q_D(ctkVTKDataSetModel);
-  this->setRowCount(0);
+
+  // Remove all items (except the first one, if there is a NULL item)
+  if (d->IncludeNullItem)
+    {
+    if (this->rowCount()<1)
+      {
+      this->insertNullItem();
+      }
+    else
+      {
+      this->setRowCount(1);
+      }
+    }
+  else
+    {
+    this->setRowCount(0);
+    }
 
   if (d->DataSet.GetPointer() == 0)
     {
@@ -318,7 +374,11 @@ void ctkVTKDataSetModel
 ::insertArray(vtkAbstractArray* array, int location, int row)
 {
   Q_D(ctkVTKDataSetModel);
-  Q_ASSERT(vtkAbstractArray::SafeDownCast(array));
+  if (vtkAbstractArray::SafeDownCast(array)==0)
+    {
+    // it is normal, it happens when arrays are pre-allocated for a data set
+    return;
+    }
 
   QList<QStandardItem*> items;
   for (int i= 0; i < this->columnCount(); ++i)
@@ -401,6 +461,36 @@ void ctkVTKDataSetModel::updateArrayFromItem(vtkAbstractArray* array, QStandardI
 void ctkVTKDataSetModel::onDataSetModified(vtkObject* dataSet)
 {
   Q_UNUSED(dataSet);
+  Q_D(ctkVTKDataSetModel);
+
+  // If a point or cell data array is added or removed then DataSet's Modified is not invoked.
+  // Therefore, we need to add observers to the point and cell data objects to make sure
+  // the list of arrays is kept up-to-date.
+
+  vtkPointData* dataSetPointData = d->DataSet ? d->DataSet->GetPointData() : 0;
+  this->qvtkReconnect(d->DataSetPointData, dataSetPointData, vtkCommand::ModifiedEvent,
+                      this, SLOT(onDataSetPointDataModified(vtkObject*)) );
+  d->DataSetPointData = dataSetPointData;
+
+  vtkCellData* dataSetCellData = d->DataSet ? d->DataSet->GetCellData() : 0;
+  this->qvtkReconnect(d->DataSetCellData, dataSetCellData, vtkCommand::ModifiedEvent,
+                      this, SLOT(onDataSetCellDataModified(vtkObject*)) );
+  d->DataSetCellData = dataSetCellData;
+
+  this->updateDataSet();
+}
+
+//------------------------------------------------------------------------------
+void ctkVTKDataSetModel::onDataSetPointDataModified(vtkObject* dataSetPointData)
+{
+  Q_UNUSED(dataSetPointData);
+  this->updateDataSet();
+}
+
+//------------------------------------------------------------------------------
+void ctkVTKDataSetModel::onDataSetCellDataModified(vtkObject* dataSetCellData)
+{
+  Q_UNUSED(dataSetCellData);
   this->updateDataSet();
 }
 
@@ -426,3 +516,44 @@ void ctkVTKDataSetModel::onItemChanged(QStandardItem * item)
   Q_ASSERT(array);
   this->updateArrayFromItem(array, item);
 }
+
+//------------------------------------------------------------------------------
+void ctkVTKDataSetModel::insertNullItem()
+{
+  QStandardItem* nullItem = new QStandardItem();
+  nullItem->setData(QVariant::fromValue(qlonglong(0)), ctkVTK::PointerRole);
+  nullItem->setData(this->NullItemLocation, ctkVTK::LocationRole);
+  nullItem->setText(QString());
+  this->insertRow(0,nullItem);
+}
+
+//------------------------------------------------------------------------------
+void ctkVTKDataSetModel::removeNullItem()
+{
+  if (this->rowCount() <= 0)
+    {
+    return;
+    }
+  // NULL item must be the first one
+  QStandardItem* nullItem = this->item(0);
+  Q_ASSERT(nullItem);
+  if (nullItem == 0)
+    {
+    return;
+    }
+  // NULL item has a special location value
+  int nullItemLocation = nullItem->data(ctkVTK::LocationRole).toInt();
+  Q_ASSERT(nullItemLocation == this->NullItemLocation);
+  if (nullItemLocation != this->NullItemLocation)
+    {
+    return;
+    }
+  // the first item is indeed the NULL item, so we remove it now
+  this->removeRow(0);
+}
+
+//------------------------------------------------------------------------------
+int ctkVTKDataSetModel::nullItemLocation()const
+{
+  return this->NullItemLocation;
+}