From 9b14efaa6dc6c36ad654e27711df30a82227b13a Mon Sep 17 00:00:00 2001 From: Max Lyon Date: Thu, 31 Oct 2019 17:56:37 +0100 Subject: [PATCH] fix property manager for mesh properties --- src/OpenMesh/Core/Utils/Property.hh | 28 ++++ src/OpenMesh/Core/Utils/PropertyManager.hh | 172 ++++++++++++++------- src/Unittests/unittests_propertymanager.cc | 65 +------- 3 files changed, 152 insertions(+), 113 deletions(-) diff --git a/src/OpenMesh/Core/Utils/Property.hh b/src/OpenMesh/Core/Utils/Property.hh index cf2fdf5c..fb97f81c 100644 --- a/src/OpenMesh/Core/Utils/Property.hh +++ b/src/OpenMesh/Core/Utils/Property.hh @@ -545,11 +545,39 @@ struct MPropHandleT : public BasePropHandleT { typedef T Value; typedef T value_type; + typedef void Handle; explicit MPropHandleT(int _idx=-1) : BasePropHandleT(_idx) {} explicit MPropHandleT(const BasePropHandleT& _b) : BasePropHandleT(_b) {} }; +template +struct PropHandle; + +template <> +struct PropHandle { + template + using type = VPropHandleT; +}; + +template <> +struct PropHandle { + template + using type = HPropHandleT; +}; + +template <> +struct PropHandle { + template + using type = EPropHandleT; +}; + +template <> +struct PropHandle { + template + using type = FPropHandleT; +}; + } // namespace OpenMesh //============================================================================= #endif // OPENMESH_PROPERTY_HH defined diff --git a/src/OpenMesh/Core/Utils/PropertyManager.hh b/src/OpenMesh/Core/Utils/PropertyManager.hh index 183a77f5..cc462fd5 100644 --- a/src/OpenMesh/Core/Utils/PropertyManager.hh +++ b/src/OpenMesh/Core/Utils/PropertyManager.hh @@ -83,6 +83,43 @@ class PropertyManager { using Value = typename PROPTYPE::Value; using value_type = typename PROPTYPE::value_type; using Handle = typename PROPTYPE::Handle; + using Self = PropertyManager; + + private: + // Mesh properties (MPropHandleT<...>) are stored differently than the other properties. + // This class implements different behavior when copying or swapping data from one + // property manager to a another one. + template + struct StorageT; + + // specialization for Mesh Properties + template + struct StorageT> { + static void copy(const PropertyManager& from, PropertyManager2& to) { + *to = *from; + } + static void swap(PropertyManager& from, PropertyManager2& to) { + std::swap(*to, *from); + } + }; + + // definition for other Mesh Properties + template + struct StorageT { + static void copy(const PropertyManager& from, PropertyManager2& to) { + from.copy_to(from.mesh_.template all_elements(), to, to.mesh_.template all_elements()); + } + static void swap(PropertyManager& lhs, PropertyManager2& rhs) { + std::swap(lhs.mesh_.property(lhs.prop_).data_vector(), rhs.mesh_.property(rhs.prop_).data_vector()); + // resize the property to the correct size + lhs.mesh_.property(lhs.prop_).resize(lhs.mesh_.template n_elements()); + rhs.mesh_.property(rhs.prop_).resize(rhs.mesh_.template n_elements()); + } + }; + + using Storage = StorageT; + + public: /** * @deprecated Use a constructor without \p existing and check existance with hasProperty() instead. @@ -140,7 +177,7 @@ class PropertyManager { * @param initial_value * @param propname The name of the property. */ - PropertyManager(const Value& intial_value, PolyConnectivity& mesh, const char *propname) : mesh_(mesh), retain_(true), name_(propname) { + PropertyManager(PolyConnectivity& mesh, const char *propname, const Value& intial_value) : mesh_(mesh), retain_(true), name_(propname) { if (!mesh_.get_property_handle(prop_, propname)) { mesh_.add_property(prop_, propname); set_range(mesh_.all_elements(), intial_value); @@ -166,7 +203,7 @@ class PropertyManager { * * @param mesh The mesh on which to create the property. */ - PropertyManager(const Value& intial_value, PolyConnectivity& mesh) : mesh_(mesh), retain_(false), name_("") { + PropertyManager(PolyConnectivity& mesh, const Value& intial_value) : mesh_(mesh), retain_(false), name_("") { mesh_.add_property(prop_, name_); set_range(mesh_.all_elements(), intial_value); } @@ -197,7 +234,7 @@ class PropertyManager { else // unnamed property -> create a property manager refering to a new property and copy the contents { mesh_.add_property(prop_, name_); - rhs.copy_to(rhs.mesh_.template all_elements(), *this, mesh_.all_elements()); + Storage::copy(rhs, *this); } } @@ -206,7 +243,7 @@ class PropertyManager { if (&mesh_ == &rhs.mesh_ && prop_ == rhs.prop_) ; // nothing to do else - rhs.copy_to(rhs.mesh_.template all_elements(), *this, mesh_.all_elements()); + Storage::copy(rhs, *this); return *this; } @@ -280,14 +317,12 @@ class PropertyManager { if (rhs.retain_) { // retained properties cannot be invalidated. Copy instead - rhs.copy_to(rhs.mesh_.template all_elements(), *this, mesh_.all_elements()); + Storage::copy(rhs, *this); } else { // switch the data stored in the properties - std::swap(mesh_.property(prop_).data_vector(), rhs.mesh_.property(rhs.prop_).data_vector()); - // resize the property to the correct size - mesh_.property(prop_).resize(mesh_.n_elements()); + Storage::swap(rhs, *this); // remove the property from rhs rhs.mesh_.remove_property(rhs.prop_); // invalidate prop_ @@ -560,6 +595,7 @@ class PropertyManager { dst_range.begin(), dst_range.end()); } + /** * Copy the values of a property from a source range to * a target range. The source range must not be smaller than the @@ -602,6 +638,8 @@ class PropertyManager { }; /** @relates PropertyManager + * + * @deprecated Temporary properties should not have a name. * * Creates a new property whose lifetime is limited to the current scope. * @@ -628,30 +666,69 @@ class PropertyManager { */ template PropertyManager::type> -makeTemporaryProperty(PolyConnectivity &mesh, const char *propname = "") { +OM_DEPRECATED("Named temporary properties are deprecated. Either create a temporary without name or a non-temporary with name") +makeTemporaryProperty(PolyConnectivity &mesh, const char *propname) { return PropertyManager::type>(mesh, propname, false); } -///// shortcut or makeTemporaryrProperty -//template -//PropertyManager::type, MeshT> -//makeTemporaryVertexProperty(MeshT &mesh, const char *propname) { return makeTemporaryProperty(mesh, propname); } +/** @relates PropertyManager + * + * Creates a new property whose lifetime is limited to the current scope. + * + * Used for temporary properties. Shadows any existing properties of + * matching name and type. + * + * Example: + * @code + * PolyMesh m; + * { + * auto is_quad = makeTemporaryProperty(m); + * for (auto& fh : m.faces()) { + * is_quad[fh] = (m.valence(fh) == 4); + * } + * // The property is automatically removed from the mesh at the end of the scope. + * } + * @endcode + * + * @param mesh The mesh on which the property is created + * @tparam ElementT Element type of the created property, e.g. VertexHandle, HalfedgeHandle, etc. + * @tparam T Value type of the created property, e.g., \p double, \p int, etc. + * @returns A PropertyManager handling the lifecycle of the property + */ +template +PropertyManager::type> +makeTemporaryProperty(PolyConnectivity &mesh) { + return PropertyManager::type>(mesh); +} -///// shortcut or makeTemporaryrProperty -//template -//PropertyManager::type, MeshT> -//makeTemporaryHalfedgeProperty(MeshT &mesh, const char *propname) { return makeTemporaryProperty(mesh, propname); } - -///// shortcut or makeTemporaryrProperty -//template -//PropertyManager::type, MeshT> -//makeTemporaryEdgeProperty(MeshT &mesh, const char *propname) { return makeTemporaryProperty(mesh, propname); } - -///// shortcut or makeTemporaryrProperty -//template -//PropertyManager::type, MeshT> -//makeTemporaryFaceProperty(MeshT &mesh, const char *propname) { return makeTemporaryProperty(mesh, propname); } +/** @relates PropertyManager + * + * Tests whether a property with the given element type, value type, and name is + * present on the given mesh. + * + * * Example: + * @code + * PolyMesh m; + * if (hasProperty(m, "is_quad")) { + * // We now know the property exists: getProperty won't throw. + * auto is_quad = getProperty(m, "is_quad"); + * // Use is_quad here. + * } + * @endcode + * + * @param mesh The mesh in question + * @param propname The property name of the expected property + * @tparam ElementT Element type of the expected property, e.g. VertexHandle, HalfedgeHandle, etc. + * @tparam T Value type of the expected property, e.g., \p double, \p int, etc. + * @tparam MeshT Type of the mesh. Can often be inferred from \p mesh + */ +template +bool +hasProperty(const PolyConnectivity &mesh, const char *propname) { + typename HandleToPropHandle::type ph; + return mesh.get_property_handle(ph, propname); +} /** @relates PropertyManager * @@ -683,7 +760,13 @@ makeTemporaryProperty(PolyConnectivity &mesh, const char *propname = "") { template PropertyManager::type> getProperty(PolyConnectivity &mesh, const char *propname) { - return PropertyManager::type>(mesh, propname, true); + if (!hasProperty(mesh, propname)) + { + std::ostringstream oss; + oss << "Requested property handle \"" << propname << "\" does not exist."; + throw std::runtime_error(oss.str()); + } + return PropertyManager::type>(mesh, propname); } /** @relates PropertyManager @@ -721,34 +804,6 @@ getOrMakeProperty(PolyConnectivity &mesh, const char *propname) { return PropertyManager::type>::createIfNotExists(mesh, propname); } -/** @relates PropertyManager - * - * Tests whether a property with the given element type, value type, and name is - * present on the given mesh. - * - * * Example: - * @code - * PolyMesh m; - * if (hasProperty(m, "is_quad")) { - * // We now know the property exists: getProperty won't throw. - * auto is_quad = getProperty(m, "is_quad"); - * // Use is_quad here. - * } - * @endcode - * - * @param mesh The mesh in question - * @param propname The property name of the expected property - * @tparam ElementT Element type of the expected property, e.g. VertexHandle, HalfedgeHandle, etc. - * @tparam T Value type of the expected property, e.g., \p double, \p int, etc. - * @tparam MeshT Type of the mesh. Can often be inferred from \p mesh - */ -template -bool -hasProperty(const PolyConnectivity &mesh, const char *propname) { - typename HandleToPropHandle::type ph; - return mesh.get_property_handle(ph, propname); -} - /** @relates PropertyManager * @deprecated Use makeTemporaryProperty() instead. * @@ -853,6 +908,8 @@ getPointsProperty(MeshT &mesh) { return PropertyManager>(mesh, mesh.points_property_handle()); } +template +using Prop = PropertyManager::template type>; template using VProp = PropertyManager>; @@ -866,6 +923,9 @@ using EProp = PropertyManager>; template using FProp = PropertyManager>; +template +using MProp = PropertyManager>; + } /* namespace OpenMesh */ #endif /* PROPERTYMANAGER_HH_ */ diff --git a/src/Unittests/unittests_propertymanager.cc b/src/Unittests/unittests_propertymanager.cc index e5046194..8263fcd5 100644 --- a/src/Unittests/unittests_propertymanager.cc +++ b/src/Unittests/unittests_propertymanager.cc @@ -130,55 +130,6 @@ TEST_F(OpenMeshPropertyManager, set_range_bool) { } } -/* - * ==================================================================== - * Factory Functions - * ==================================================================== - */ - -/* - * Temporary property - */ -TEST_F(OpenMeshPropertyManager, cpp11_temp_property) { - using handle_type = OpenMesh::VPropHandleT; - const auto prop_name = "pm_v_test_property"; - ASSERT_FALSE((OpenMesh::hasProperty(mesh_, prop_name))); - - { - auto vprop = OpenMesh::makeTemporaryProperty(mesh_, prop_name); - static_cast(vprop); // Unused variable - ASSERT_TRUE((OpenMesh::hasProperty(mesh_, prop_name))); - } - - ASSERT_FALSE((OpenMesh::hasProperty(mesh_, prop_name))); -} - -/* - * Two temporary properties on a mesh using the same name and type. The second - * (inner) one shadows the first (outer) one instead of aliasing. - */ -TEST_F(OpenMeshPropertyManager, cpp11_temp_property_shadowing) { - auto vh = mesh_.add_vertex({0,0,0}); // Dummy vertex to attach properties to - - const auto prop_name = "pm_v_test_property"; - - auto outer_prop = OpenMesh::makeTemporaryProperty(mesh_, prop_name); - outer_prop[vh] = 100; - ASSERT_EQ(100, outer_prop[vh]); - - { - // inner_prop uses same type and name as outer_prop - auto inner_prop = OpenMesh::makeTemporaryProperty(mesh_, prop_name); - inner_prop[vh] = 200; - ASSERT_EQ(200, inner_prop[vh]); - // End of scope: inner_prop is removed from mesh_ - } - - // Ensure outer_prop still exists and its data has not been overwritten by inner_prop - ASSERT_TRUE((OpenMesh::hasProperty(mesh_, prop_name))); - ASSERT_EQ(100, outer_prop[vh]); -} - /* * In sequence: * - add a persistent property to a mesh @@ -315,8 +266,8 @@ TEST_F(OpenMeshPropertyManager, property_copying_same_mesh) { // unnamed to unnamed { - auto prop1 = OpenMesh::VProp(3, mesh_); - auto prop2 = OpenMesh::VProp(0, mesh_); + auto prop1 = OpenMesh::VProp(mesh_, 3); + auto prop2 = OpenMesh::VProp(mesh_, 0); EXPECT_EQ(prop1[OpenMesh::VertexHandle(0)], 3) << "Property not initialized correctly"; EXPECT_EQ(prop2[OpenMesh::VertexHandle(0)], 0) << "Property not initialized correctly"; @@ -341,7 +292,7 @@ TEST_F(OpenMeshPropertyManager, property_copying_same_mesh) { // unnamed to named { auto prop1 = OpenMesh::VProp(mesh_); - auto prop2 = OpenMesh::VProp(0, mesh_, "ids"); + auto prop2 = OpenMesh::VProp(mesh_, "ids", 0); EXPECT_EQ(prop2[OpenMesh::VertexHandle(0)], 0) << "Property not initialized correctly"; for (auto vh : mesh_.vertices()) @@ -579,8 +530,8 @@ TEST_F(OpenMeshPropertyManager, property_copying_different_mesh) { // unnamed to unnamed { - auto prop1 = OpenMesh::VProp(3, mesh_); - auto prop2 = OpenMesh::VProp(0, copy); + auto prop1 = OpenMesh::VProp(mesh_, 3); + auto prop2 = OpenMesh::VProp(copy, 0); EXPECT_EQ(prop1[OpenMesh::VertexHandle(0)], 3) << "Property not initialized correctly"; EXPECT_EQ(prop2[OpenMesh::VertexHandle(0)], 0) << "Property not initialized correctly"; @@ -600,13 +551,13 @@ TEST_F(OpenMeshPropertyManager, property_copying_different_mesh) { EXPECT_EQ(prop1[OpenMesh::VertexHandle(0)], 0) << "Property not copied correctly"; EXPECT_EQ(prop2[OpenMesh::VertexHandle(0)], -13) << "Property not copied correctly"; - EXPECT_NO_FATAL_FAILURE(prop2[OpenMesh::VertexHandle(copy.n_vertices()-1)]) << "Property not correctly resized"; + EXPECT_NO_FATAL_FAILURE(prop2[OpenMesh::VertexHandle(static_cast(copy.n_vertices())-1)]) << "Property not correctly resized"; } // unnamed to named { auto prop1 = OpenMesh::VProp(mesh_); - auto prop2 = OpenMesh::VProp(0, copy, "ids"); + auto prop2 = OpenMesh::VProp(copy, "ids", 0); EXPECT_EQ(prop2[OpenMesh::VertexHandle(0)], 0) << "Property not initialized correctly"; for (auto vh : mesh_.vertices()) @@ -731,7 +682,7 @@ TEST_F(OpenMeshPropertyManager, property_moving_different_mesh) { EXPECT_FALSE(prop1.isValid()) << "prop1 not invalidated after moving"; EXPECT_EQ(prop2[OpenMesh::VertexHandle(0)], -13) << "Property not copied correctly"; - EXPECT_NO_FATAL_FAILURE(prop2[OpenMesh::VertexHandle(copy.n_vertices()-1)]) << "Property not correctly resized"; + EXPECT_NO_FATAL_FAILURE(prop2[OpenMesh::VertexHandle(static_cast(copy.n_vertices())-1)]) << "Property not correctly resized"; } // unnamed to named