From 35bbc1d09749594ad908f48a6cb3d0e31a4ff834 Mon Sep 17 00:00:00 2001 From: Niels Dekker Date: Tue, 29 Oct 2024 18:00:57 +0100 Subject: [PATCH 1/2] ENH: Let `PointSet::Clone()` do a "deep copy", instead of no copy at all Implemented InternalClone() for PointSet and PointSetBase, in order to let `PointSet::Clone()` and `PointSetBase::Clone()` do a deep copy of its points and point data. --- Modules/Core/Common/include/itkPointSet.h | 3 + Modules/Core/Common/include/itkPointSet.hxx | 17 ++++++ Modules/Core/Common/include/itkPointSetBase.h | 5 ++ .../Core/Common/include/itkPointSetBase.hxx | 25 ++++++++ Modules/Core/Common/test/itkPointSetGTest.cxx | 57 +++++++++++++++++++ 5 files changed, 107 insertions(+) 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::Graft(const DataObject * data) this->SetPointData(pointSet->m_PointDataContainer); } +template +LightObject::Pointer +PointSet::InternalClone() const +{ + LightObject::Pointer lightObject = Superclass::InternalClone(); + + if (auto * const clone = dynamic_cast(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::VerifyRequestedRegion() return retval; } +template +LightObject::Pointer +PointSetBase::InternalClone() const +{ + LightObject::Pointer lightObject = Superclass::InternalClone(); + + if (auto * const clone = dynamic_cast(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 PointSetBase::~PointSetBase() = default; 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 +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::New()); + + // Then check a non-empty 2-D point set with `double` data: + using PixelType = double; + using PointSetType = itk::PointSet; + using PointType = PointSetType::PointType; + + const auto pointSet = PointSetType::New(); + pointSet->SetPoints(itk::MakeVectorContainer({ PointType(), itk::MakeFilled(1.0f) })); + pointSet->SetPointData(itk::MakeVectorContainer({ 0.0, 1.0, 2.0 })); + + TestCloneDoesDeepCopy(*pointSet); +} From 0d78aae5df7fb17532f2d47869d16e836468fe3c Mon Sep 17 00:00:00 2001 From: Niels Dekker Date: Wed, 6 Nov 2024 15:20:51 +0100 Subject: [PATCH 2/2] DOC: Note in Migration Guide that PointSet::Clone() now copies its data - Addressed a comment by Matt McCormick at https://github.com/InsightSoftwareConsortium/ITK/pull/4923#issuecomment-2457473255 --- .../docs/migration_guides/itk_6_migration_guide.md | 10 ++++++++++ 1 file changed, 10 insertions(+) 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.)