From 48c783c2c503c8ede04ecb4a7f1cc34aeff8b540 Mon Sep 17 00:00:00 2001 From: Marius Kintel Date: Tue, 12 Mar 2024 00:46:20 -0400 Subject: [PATCH] Add constResult()/mutableResult() for explicit result creation --- src/geometry/GeometryEvaluator.cc | 24 +++++++++---------- src/geometry/GeometryEvaluator.h | 14 +++++++++-- src/geometry/boolean_utils.cc | 14 ++++++----- .../manifold/manifold-applyops-minkowski.cc | 2 +- src/geometry/manifold/manifold-applyops.cc | 2 +- src/geometry/manifold/manifoldutils.cc | 10 ++++---- src/geometry/manifold/manifoldutils.h | 7 +++--- 7 files changed, 43 insertions(+), 30 deletions(-) diff --git a/src/geometry/GeometryEvaluator.cc b/src/geometry/GeometryEvaluator.cc index da1d089e20..012944c383 100644 --- a/src/geometry/GeometryEvaluator.cc +++ b/src/geometry/GeometryEvaluator.cc @@ -108,7 +108,7 @@ GeometryEvaluator::ResultObject GeometryEvaluator::applyToChildren(const Abstrac for (const auto& item : this->visitedchildren[node.index()]) { if (!isValidDim(item, dim)) break; } - if (dim == 2) return {std::shared_ptr(applyToChildren2D(node, op))}; + if (dim == 2) return ResultObject::mutableResult(std::shared_ptr(applyToChildren2D(node, op))); else if (dim == 3) return applyToChildren3D(node, op); return {}; } @@ -124,7 +124,7 @@ GeometryEvaluator::ResultObject GeometryEvaluator::applyToChildren3D(const Abstr if (children.empty()) return {}; if (op == OpenSCADOperator::HULL) { - return {std::shared_ptr(applyHull(children))}; + return ResultObject::mutableResult(std::shared_ptr(applyHull(children))); } else if (op == OpenSCADOperator::FILL) { for (const auto& item : children) { LOG(message_group::Warning, item.first->modinst->location(), this->tree.getDocumentPath(), "fill() not yet implemented for 3D"); @@ -132,7 +132,7 @@ GeometryEvaluator::ResultObject GeometryEvaluator::applyToChildren3D(const Abstr } // Only one child -> this is a noop - if (children.size() == 1) return {children.front().second}; + if (children.size() == 1) return ResultObject::constResult(children.front().second); switch (op) { case OpenSCADOperator::MINKOWSKI: @@ -142,8 +142,8 @@ GeometryEvaluator::ResultObject GeometryEvaluator::applyToChildren3D(const Abstr if (item.second && !item.second->isEmpty()) actualchildren.push_back(item); } if (actualchildren.empty()) return {}; - if (actualchildren.size() == 1) return {actualchildren.front().second}; - return {applyMinkowski(actualchildren)}; + if (actualchildren.size() == 1) return ResultObject::constResult(actualchildren.front().second); + return ResultObject::constResult(applyMinkowski(actualchildren)); break; } case OpenSCADOperator::UNION: @@ -153,17 +153,17 @@ GeometryEvaluator::ResultObject GeometryEvaluator::applyToChildren3D(const Abstr if (item.second && !item.second->isEmpty()) actualchildren.push_back(item); } if (actualchildren.empty()) return {}; - if (actualchildren.size() == 1) return {actualchildren.front().second}; + if (actualchildren.size() == 1) return ResultObject::constResult(actualchildren.front().second); #ifdef ENABLE_MANIFOLD if (Feature::ExperimentalManifold.is_enabled()) { - return {ManifoldUtils::applyOperator3DManifold(actualchildren, op)}; + return ResultObject::mutableResult(ManifoldUtils::applyOperator3DManifold(actualchildren, op)); } #endif #ifdef ENABLE_CGAL else if (Feature::ExperimentalFastCsg.is_enabled()) { - return {std::shared_ptr(CGALUtils::applyUnion3DHybrid(actualchildren.begin(), actualchildren.end()))}; + return ResultObject::mutableResult(std::shared_ptr(CGALUtils::applyUnion3DHybrid(actualchildren.begin(), actualchildren.end()))); } - return {CGALUtils::applyUnion3D(actualchildren.begin(), actualchildren.end())}; + return ResultObject::constResult(std::shared_ptr(CGALUtils::applyUnion3D(actualchildren.begin(), actualchildren.end()))); #else assert(false && "No boolean backend available"); #endif @@ -173,15 +173,15 @@ GeometryEvaluator::ResultObject GeometryEvaluator::applyToChildren3D(const Abstr { #ifdef ENABLE_MANIFOLD if (Feature::ExperimentalManifold.is_enabled()) { - return {ManifoldUtils::applyOperator3DManifold(children, op)}; + return ResultObject::mutableResult(ManifoldUtils::applyOperator3DManifold(children, op)); } #endif #ifdef ENABLE_CGAL if (Feature::ExperimentalFastCsg.is_enabled()) { // FIXME: It's annoying to have to disambiguate here: - return {std::shared_ptr(CGALUtils::applyOperator3DHybrid(children, op))}; + return ResultObject::mutableResult(std::shared_ptr(CGALUtils::applyOperator3DHybrid(children, op))); } - return {CGALUtils::applyOperator3D(children, op)}; + return ResultObject::constResult(CGALUtils::applyOperator3D(children, op)); #else assert(false && "No boolean backend available"); #endif diff --git a/src/geometry/GeometryEvaluator.h b/src/geometry/GeometryEvaluator.h index 6c2793ab8f..379cc84f91 100644 --- a/src/geometry/GeometryEvaluator.h +++ b/src/geometry/GeometryEvaluator.h @@ -50,11 +50,18 @@ class GeometryEvaluator : public NodeVisitor class ResultObject { public: + // This makes it explicit if we want a const vs. non-const result. + // This is important to avoid inadvertently tagging a geometry as const when + // the underlying geometry is actually mutable. + // The template trick, combined with private constructors, makes it possible + // to create a ResultObject containing a const, _only_ from const objects + // (i.e. no implicit conversion from non-const to const). + template static ResultObject constResult(std::shared_ptr geom) {return {geom};} + template static ResultObject mutableResult(std::shared_ptr geom) {return {geom};} + // Default constructor with nullptr can be used to represent empty geometry, // for example union() with no children, etc. ResultObject() : is_const(true) {} - ResultObject(std::shared_ptr g) : is_const(true), const_pointer(std::move(g)) {} - ResultObject(std::shared_ptr g) : is_const(false), pointer(std::move(g)) {} std::shared_ptr ptr() { assert(!is_const); return pointer; } [[nodiscard]] std::shared_ptr constptr() const { return is_const ? const_pointer : std::static_pointer_cast(pointer); @@ -64,6 +71,9 @@ class GeometryEvaluator : public NodeVisitor else return ptr(); } private: + template ResultObject(std::shared_ptr g) : is_const(true), const_pointer(std::move(g)) {} + template ResultObject(std::shared_ptr g) : is_const(false), pointer(std::move(g)) {} + bool is_const; std::shared_ptr pointer; std::shared_ptr const_pointer; diff --git a/src/geometry/boolean_utils.cc b/src/geometry/boolean_utils.cc index b8032aa10f..59971194db 100644 --- a/src/geometry/boolean_utils.cc +++ b/src/geometry/boolean_utils.cc @@ -11,11 +11,11 @@ #include #include #include "cgalutils.h" -#endif +#endif // ENABLE_CGAL #ifdef ENABLE_MANIFOLD #include "ManifoldGeometry.h" #include "manifoldutils.h" -#endif +#endif // ENABLE_MANIFOLD #include "Feature.h" #include "PolySet.h" @@ -65,7 +65,7 @@ std::unique_ptr applyHull(const Geometry::Geometries& children) addPoint(CGALUtils::vector_convert(p)); return false; }); -#endif +#endif // ENABLE_MANIFOLD } else if (const auto *ps = dynamic_cast(chgeom.get())) { addCapacity(ps->indices.size() * 3); for (const auto& p : ps->indices) { @@ -100,6 +100,8 @@ std::unique_ptr applyHull(const Geometry::Geometries& children) /*! children cannot contain nullptr objects + + FIXME: This shouldn't return const, but it does due to internal implementation details */ std::shared_ptr applyMinkowski(const Geometry::Geometries& children) { @@ -107,7 +109,7 @@ std::shared_ptr applyMinkowski(const Geometry::Geometries& child if (Feature::ExperimentalManifold.is_enabled()) { return ManifoldUtils::applyMinkowskiManifold(children); } -#endif +#endif // ENABLE_MANIFOLD if (Feature::ExperimentalFastCsg.is_enabled()) { return CGALUtils::applyMinkowskiHybrid(children); } @@ -311,7 +313,7 @@ std::shared_ptr applyMinkowski(const Geometry::Geometries& child return N; } } -#else +#else // ENABLE_CGAL bool applyHull(const Geometry::Geometries& children, PolySet& result) { return false; @@ -321,4 +323,4 @@ std::shared_ptr applyMinkowski(const Geometry::Geometries& child { return std::make_shared(3); } -#endif +#endif // ENABLE_CGAL diff --git a/src/geometry/manifold/manifold-applyops-minkowski.cc b/src/geometry/manifold/manifold-applyops-minkowski.cc index 0c5d61e88c..ddc866e77b 100644 --- a/src/geometry/manifold/manifold-applyops-minkowski.cc +++ b/src/geometry/manifold/manifold-applyops-minkowski.cc @@ -119,7 +119,7 @@ std::shared_ptr applyMinkowskiManifold(const Geometry::Geometrie if (minkowski_points.size() <= 3) { t.stop(); - return std::make_shared(); + return std::make_shared(); } t.stop(); diff --git a/src/geometry/manifold/manifold-applyops.cc b/src/geometry/manifold/manifold-applyops.cc index 6b84bc0818..cebbb4144f 100644 --- a/src/geometry/manifold/manifold-applyops.cc +++ b/src/geometry/manifold/manifold-applyops.cc @@ -20,7 +20,7 @@ Location getLocation(const std::shared_ptr& node) Applies op to all children and returns the result. The child list should be guaranteed to contain non-NULL 3D or empty Geometry objects */ -std::shared_ptr applyOperator3DManifold(const Geometry::Geometries& children, OpenSCADOperator op) +std::shared_ptr applyOperator3DManifold(const Geometry::Geometries& children, OpenSCADOperator op) { std::shared_ptr geom; diff --git a/src/geometry/manifold/manifoldutils.cc b/src/geometry/manifold/manifoldutils.cc index 210664e3ac..389101a0d3 100644 --- a/src/geometry/manifold/manifoldutils.cc +++ b/src/geometry/manifold/manifoldutils.cc @@ -66,7 +66,7 @@ std::shared_ptr trustedPolySetToManifold(const PolySet& ps) } template -std::shared_ptr createManifoldFromSurfaceMesh(const TriangleMesh& tm) +std::shared_ptr createManifoldFromSurfaceMesh(const TriangleMesh& tm) { typedef typename TriangleMesh::Vertex_index vertex_descriptor; @@ -102,8 +102,8 @@ std::shared_ptr createManifoldFromSurfaceMesh(const Tria } #ifdef ENABLE_CGAL -template std::shared_ptr createManifoldFromSurfaceMesh(const CGAL::Surface_mesh> &tm); -template std::shared_ptr createManifoldFromSurfaceMesh(const CGAL_DoubleMesh &tm); +template std::shared_ptr createManifoldFromSurfaceMesh(const CGAL::Surface_mesh> &tm); +template std::shared_ptr createManifoldFromSurfaceMesh(const CGAL_DoubleMesh &tm); #endif std::unique_ptr createManifoldFromTriangularPolySet(const PolySet& ps) @@ -126,7 +126,7 @@ std::unique_ptr createManifoldFromTriangularPolySet(co return std::make_unique(std::move(mesh)); } -std::shared_ptr createManifoldFromPolySet(const PolySet& ps) +std::shared_ptr createManifoldFromPolySet(const PolySet& ps) { // 1. If the PolySet is already manifold, we should be able to build a Manifold object directly // (through using manifold::Mesh). @@ -141,7 +141,7 @@ std::shared_ptr createManifoldFromPolySet(const PolySet& auto mani = createManifoldFromTriangularPolySet(triangle_set); if (mani->Status() == Error::NoError) { - return std::make_shared(std::shared_ptr(std::move(mani))); + return std::make_shared(std::shared_ptr(std::move(mani))); } // FIXME: Should we suppress this warning, as it may not be very actionable? diff --git a/src/geometry/manifold/manifoldutils.h b/src/geometry/manifold/manifoldutils.h index 3a23d61cf5..b602c0aba6 100644 --- a/src/geometry/manifold/manifoldutils.h +++ b/src/geometry/manifold/manifoldutils.h @@ -19,15 +19,16 @@ namespace ManifoldUtils { /*! If the PolySet isn't trusted, use createManifoldFromPolySet which will triangulate and reorient it. */ std::shared_ptr trustedPolySetToManifold(const PolySet& ps); - std::shared_ptr createManifoldFromPolySet(const PolySet& ps); + std::shared_ptr createManifoldFromPolySet(const PolySet& ps); std::shared_ptr createManifoldFromGeometry(const std::shared_ptr& geom); template - std::shared_ptr createManifoldFromSurfaceMesh(const TriangleMesh& mesh); + std::shared_ptr createManifoldFromSurfaceMesh(const TriangleMesh& mesh); - std::shared_ptr applyOperator3DManifold(const Geometry::Geometries& children, OpenSCADOperator op); + std::shared_ptr applyOperator3DManifold(const Geometry::Geometries& children, OpenSCADOperator op); #ifdef ENABLE_CGAL + // FIXME: This shouldn't return const, but it does due to internal implementation details. std::shared_ptr applyMinkowskiManifold(const Geometry::Geometries& children); #endif };