Repository: InsightSoftwareConsortium/ITK (language: python)

Issue:
ENH: Add Clone() and MakeDeepCopy() to PointSetBase
`PointSetBase::MakeDeepCopy()` allows making a "deep copy" of a point set (rather than `PointSet::Graft`, which does a "shallow copy", or `Clone()`, which does not copy at all).

----

Background: for elastix (ITK/Python interface), we would like to be able to make a "deep copy" of a point set, including its points and point data. Unfortunately `PointSet::Clone()` doesn't do it. It does not copy anything at all!!! An alternative solution would be to adjust `PointSet::Clone()`, so that it would do the deep copy. Would that be better? What do you think?


Discussion / hints:

Fix patch:
diff --git a/Documentation/docs/migration_guides/itk_6_migration_guide.md b/Documentation/docs/migration_guides/itk_6_migration_guide.md
index 83e6588c3f2..fbc47e17cde 100644
--- a/Documentation/docs/migration_guides/itk_6_migration_guide.md
+++ b/Documentation/docs/migration_guides/itk_6_migration_guide.md
@@ -97,3 +97,13 @@ Accessing outdated ITKv5 migration scripts
 git worktree add .../ITKv5.4 v5.4.0
 ls ../ITKv5/Utilities/ITKv5Preparation
 ```
+
+Class changes
+-------------
+
+The `Clone()` member function of `itk::PointSet` now does a "deep copy" of its
+data, creating a new instance that has a copy of the points, the point data and
+the region information properties of the original PointSet object. With previous
+ITK versions, `PointSet::Clone()` did not copy any data. (It previously just
+created a default-constructed PointSet object, like `PointSet::CreateAnother()`
+does.)
diff --git a/Modules/Core/Common/include/itkPointSet.h b/Modules/Core/Common/include/itkPointSet.h
index 51829cbd6c7..05e2040675a 100644
--- a/Modules/Core/Common/include/itkPointSet.h
+++ b/Modules/Core/Common/include/itkPointSet.h
@@ -162,6 +162,9 @@ class ITK_TEMPLATE_EXPORT PointSet : public PointSetBase<typename TMeshTraits::P
   void
   PrintSelf(std::ostream & os, Indent indent) const override;
 
+  LightObject::Pointer
+  InternalClone() const override;
+
 }; // End Class: PointSet
 } // end namespace itk
 
diff --git a/Modules/Core/Common/include/itkPointSet.hxx b/Modules/Core/Common/include/itkPointSet.hxx
index 7af832d5406..bba950ade26 100644
--- a/Modules/Core/Common/include/itkPointSet.hxx
+++ b/Modules/Core/Common/include/itkPointSet.hxx
@@ -143,6 +143,23 @@ PointSet<TPixelType, VDimension, TMeshTraits>::Graft(const DataObject * data)
   this->SetPointData(pointSet->m_PointDataContainer);
 }
 
+template <typename TPixelType, unsigned int VDimension, typename TMeshTraits>
+LightObject::Pointer
+PointSet<TPixelType, VDimension, TMeshTraits>::InternalClone() const
+{
+  LightObject::Pointer lightObject = Superclass::InternalClone();
+
+  if (auto * const clone = dynamic_cast<Self *>(lightObject.GetPointer()))
+  {
+    if (m_PointDataContainer)
+    {
+      clone->m_PointDataContainer = PointDataContainer::New();
+      clone->m_PointDataContainer->CastToSTLContainer() = m_PointDataContainer->CastToSTLConstContainer();
+    }
+    return lightObject;
+  }
+  itkExceptionMacro("downcast to type " << this->GetNameOfClass() << " failed.");
+}
 } // end namespace itk
 
 #endif
diff --git a/Modules/Core/Common/include/itkPointSetBase.h b/Modules/Core/Common/include/itkPointSetBase.h
index d8c86ee9900..4b3b118229f 100644
--- a/Modules/Core/Common/include/itkPointSetBase.h
+++ b/Modules/Core/Common/include/itkPointSetBase.h
@@ -67,6 +67,8 @@ class ITK_TEMPLATE_EXPORT PointSetBase : public DataObject
   /** \see LightObject::GetNameOfClass() */
   itkOverrideGetNameOfClassMacro(PointSetBase);
 
+  itkCloneMacro(Self);
+
   /** Convenient type alias obtained from TPointsContainer template parameter. */
   using PointType = typename TPointsContainer::Element;
   using CoordRepType = typename PointType::CoordRepType;
@@ -200,6 +202,9 @@ class ITK_TEMPLATE_EXPORT PointSetBase : public DataObject
   void
   PrintSelf(std::ostream & os, Indent indent) const override;
 
+  LightObject::Pointer
+  InternalClone() const override;
+
   // If the RegionType is ITK_UNSTRUCTURED_REGION, then the following
   // variables represent the maximum number of region that the data
   // object can be broken into, which region out of how many is
diff --git a/Modules/Core/Common/include/itkPointSetBase.hxx b/Modules/Core/Common/include/itkPointSetBase.hxx
index 0826eb38144..ac364664f06 100644
--- a/Modules/Core/Common/include/itkPointSetBase.hxx
+++ b/Modules/Core/Common/include/itkPointSetBase.hxx
@@ -350,6 +350,31 @@ PointSetBase<TPointsContainer>::VerifyRequestedRegion()
   return retval;
 }
 
+template <typename TPointsContainer>
+LightObject::Pointer
+PointSetBase<TPointsContainer>::InternalClone() const
+{
+  LightObject::Pointer lightObject = Superclass::InternalClone();
+
+  if (auto * const clone = dynamic_cast<Self *>(lightObject.GetPointer()))
+  {
+    if (m_PointsContainer)
+    {
+      clone->m_PointsContainer = TPointsContainer::New();
+      clone->m_PointsContainer->CastToSTLContainer() = m_PointsContainer->CastToSTLConstContainer();
+    }
+
+    clone->m_MaximumNumberOfRegions = m_MaximumNumberOfRegions;
+    clone->m_NumberOfRegions = m_NumberOfRegions;
+    clone->m_RequestedNumberOfRegions = m_RequestedNumberOfRegions;
+    clone->m_BufferedRegion = m_BufferedRegion;
+    clone->m_RequestedRegion = m_RequestedRegion;
+
+    return lightObject;
+  }
+  itkExceptionMacro("downcast to type " << this->GetNameOfClass() << " failed.");
+}
+
 // Destructor. Must be defined here (rather than inside the class definition) because it is pure virtual.
 template <typename TPointsContainer>
 PointSetBase<TPointsContainer>::~PointSetBase() = default;


Test patch:
diff --git a/Modules/Core/Common/test/itkPointSetGTest.cxx b/Modules/Core/Common/test/itkPointSetGTest.cxx
index 1a5d305aa65..f06e8b8a2c6 100644
--- a/Modules/Core/Common/test/itkPointSetGTest.cxx
+++ b/Modules/Core/Common/test/itkPointSetGTest.cxx
@@ -73,6 +73,44 @@ TestSetPointsByCoordinates(TPointSet & pointSet)
     }
   }
 }
+
+
+template <typename TPointSet>
+void
+TestCloneDoesDeepCopy(const TPointSet & pointSet)
+{
+  const auto clone = pointSet.Clone();
+
+  // Using a const reference to the clone, because the non-const overloads of GetPointData() and GetPoints() might
+  // potentially modify the point set! (Specifically when the container pointers were initially null.)
+  const TPointSet & constClone = itk::Deref(clone.get());
+
+  if (const auto * const pointData = pointSet.GetPointData())
+  {
+    const auto * const clonedPointData = constClone.GetPointData();
+
+    ASSERT_NE(clonedPointData, nullptr);
+    EXPECT_NE(clonedPointData, pointData) << "It should not just be a shallow copy!";
+    EXPECT_EQ(clonedPointData->CastToSTLConstContainer(), pointData->CastToSTLConstContainer());
+  }
+  else
+  {
+    EXPECT_EQ(constClone.GetPointData(), nullptr);
+  }
+
+  if (const auto * const points = pointSet.GetPoints())
+  {
+    const auto * const clonedPoints = constClone.GetPoints();
+
+    ASSERT_NE(clonedPoints, nullptr);
+    EXPECT_NE(clonedPoints, points) << "It should not just be a shallow copy!";
+    EXPECT_EQ(clonedPoints->CastToSTLConstContainer(), points->CastToSTLConstContainer());
+  }
+  else
+  {
+    EXPECT_EQ(constClone.GetPoints(), nullptr);
+  }
+}
 } // namespace
 
 
@@ -113,3 +151,22 @@ TEST(PointSet, GraftDoesShallowCopy)
 
   check(*pointSet);
 }
+
+
+// Tests that `PointSet::Clone` copies the points and the data. So it does a "deep copy".
+TEST(PointSet, CloneDoesDeepCopy)
+{
+  // First check an empty point set:
+  TestCloneDoesDeepCopy(*itk::PointSet<int>::New());
+
+  // Then check a non-empty 2-D point set with `double` data:
+  using PixelType = double;
+  using PointSetType = itk::PointSet<PixelType, 2>;
+  using PointType = PointSetType::PointType;
+
+  const auto pointSet = PointSetType::New();
+  pointSet->SetPoints(itk::MakeVectorContainer<PointType>({ PointType(), itk::MakeFilled<PointType>(1.0f) }));
+  pointSet->SetPointData(itk::MakeVectorContainer<PixelType>({ 0.0, 1.0, 2.0 }));
+
+  TestCloneDoesDeepCopy(*pointSet);
+}