Add constResult()/mutableResult() for explicit result creation

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<Geometry>(applyToChildren2D(node, op))};
+  if (dim == 2) return ResultObject::mutableResult(std::shared_ptr<Geometry>(applyToChildren2D(node, op)));
   else if (dim == 3) return applyToChildren3D(node, op);
   return {};
 }
@@ -124,15 +124,15 @@ GeometryEvaluator::ResultObject GeometryEvaluator::applyToChildren3D(const Abstr
   if (children.empty()) return {};
 
   if (op == OpenSCADOperator::HULL) {
-    return {std::shared_ptr<Geometry>(applyHull(children))};
+    return ResultObject::mutableResult(std::shared_ptr<Geometry>(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");
     }
   }
 
   // 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<Geometry>(CGALUtils::applyUnion3DHybrid(actualchildren.begin(), actualchildren.end()))};
+      return ResultObject::mutableResult(std::shared_ptr<Geometry>(CGALUtils::applyUnion3DHybrid(actualchildren.begin(), actualchildren.end())));
     }
-    return {CGALUtils::applyUnion3D(actualchildren.begin(), actualchildren.end())};
+    return ResultObject::constResult(std::shared_ptr<const Geometry>(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<Geometry>(CGALUtils::applyOperator3DHybrid(children, op))};
+      return ResultObject::mutableResult(std::shared_ptr<Geometry>(CGALUtils::applyOperator3DHybrid(children, op)));
     }
-    return {CGALUtils::applyOperator3D(children, op)};
+    return ResultObject::constResult(CGALUtils::applyOperator3D(children, op));
 #else
     assert(false && "No boolean backend available");
     #endif

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<class T> static ResultObject constResult(std::shared_ptr<const T> geom) {return {geom};}
+    template<class T> static ResultObject mutableResult(std::shared_ptr<T> 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<const Geometry> g) : is_const(true), const_pointer(std::move(g)) {}
-    ResultObject(std::shared_ptr<Geometry> g) : is_const(false), pointer(std::move(g)) {}
     std::shared_ptr<Geometry> ptr() { assert(!is_const); return pointer; }
     [[nodiscard]] std::shared_ptr<const Geometry> constptr() const {
       return is_const ? const_pointer : std::static_pointer_cast<const Geometry>(pointer);
@@ -64,6 +71,9 @@ class GeometryEvaluator : public NodeVisitor
       else return ptr();
     }
 private:
+    template<class T> ResultObject(std::shared_ptr<const T> g) : is_const(true), const_pointer(std::move(g)) {}
+    template<class T> ResultObject(std::shared_ptr<T> g) : is_const(false), pointer(std::move(g)) {}
+
     bool is_const;
     std::shared_ptr<Geometry> pointer;
     std::shared_ptr<const Geometry> const_pointer;

src/geometry/boolean_utils.cc

@@ -11,11 +11,11 @@
 #include <CGAL/version.h>
 #include <CGAL/convex_hull_3.h>
 #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<PolySet> applyHull(const Geometry::Geometries& children)
           addPoint(CGALUtils::vector_convert<K::Point_3>(p));
           return false;
         });
-#endif
+#endif  // ENABLE_MANIFOLD
     } else if (const auto *ps = dynamic_cast<const PolySet*>(chgeom.get())) {
       addCapacity(ps->indices.size() * 3);
       for (const auto& p : ps->indices) {
@@ -100,14 +100,16 @@ std::unique_ptr<PolySet> 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<const Geometry> applyMinkowski(const Geometry::Geometries& children)
 {
 #if ENABLE_MANIFOLD
   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<const Geometry> 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<const Geometry> applyMinkowski(const Geometry::Geometries& child
 {
   return std::make_shared<PolySet>(3);
 }
-#endif
+#endif  // ENABLE_CGAL

src/geometry/manifold/manifold-applyops-minkowski.cc

@@ -119,7 +119,7 @@ std::shared_ptr<const Geometry> applyMinkowskiManifold(const Geometry::Geometrie
 
         if (minkowski_points.size() <= 3) {
           t.stop();
-          return std::make_shared<const ManifoldGeometry>();
+          return std::make_shared<ManifoldGeometry>();
         }
 
         t.stop();

src/geometry/manifold/manifold-applyops.cc

@@ -20,7 +20,7 @@ Location getLocation(const std::shared_ptr<const AbstractNode>& 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<const ManifoldGeometry> applyOperator3DManifold(const Geometry::Geometries& children, OpenSCADOperator op)
+std::shared_ptr<ManifoldGeometry> applyOperator3DManifold(const Geometry::Geometries& children, OpenSCADOperator op)
 {
   std::shared_ptr<ManifoldGeometry> geom;
 

src/geometry/manifold/manifoldutils.cc

@@ -66,7 +66,7 @@ std::shared_ptr<manifold::Manifold> trustedPolySetToManifold(const PolySet& ps)
 }
 
 template <class TriangleMesh>
-std::shared_ptr<const ManifoldGeometry> createManifoldFromSurfaceMesh(const TriangleMesh& tm)
+std::shared_ptr<ManifoldGeometry> createManifoldFromSurfaceMesh(const TriangleMesh& tm)
 {
   typedef typename TriangleMesh::Vertex_index vertex_descriptor;
 
@@ -102,8 +102,8 @@ std::shared_ptr<const ManifoldGeometry> createManifoldFromSurfaceMesh(const Tria
 }
 
 #ifdef ENABLE_CGAL
-template std::shared_ptr<const ManifoldGeometry> createManifoldFromSurfaceMesh(const CGAL::Surface_mesh<CGAL::Point_3<CGAL::Epick>> &tm);
-template std::shared_ptr<const ManifoldGeometry> createManifoldFromSurfaceMesh(const CGAL_DoubleMesh &tm);
+template std::shared_ptr<ManifoldGeometry> createManifoldFromSurfaceMesh(const CGAL::Surface_mesh<CGAL::Point_3<CGAL::Epick>> &tm);
+template std::shared_ptr<ManifoldGeometry> createManifoldFromSurfaceMesh(const CGAL_DoubleMesh &tm);
 #endif
 
 std::unique_ptr<const manifold::Manifold> createManifoldFromTriangularPolySet(const PolySet& ps)
@@ -126,7 +126,7 @@ std::unique_ptr<const manifold::Manifold> createManifoldFromTriangularPolySet(co
   return std::make_unique<manifold::Manifold>(std::move(mesh));
 }
 
-std::shared_ptr<const ManifoldGeometry> createManifoldFromPolySet(const PolySet& ps)
+std::shared_ptr<ManifoldGeometry> 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<const ManifoldGeometry> createManifoldFromPolySet(const PolySet&
 
   auto mani = createManifoldFromTriangularPolySet(triangle_set);
   if (mani->Status() == Error::NoError) {    
-    return std::make_shared<const ManifoldGeometry>(std::shared_ptr<const manifold::Manifold>(std::move(mani)));
+    return std::make_shared<ManifoldGeometry>(std::shared_ptr<const manifold::Manifold>(std::move(mani)));
   }
 
   // FIXME: Should we suppress this warning, as it may not be very actionable?

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<manifold::Manifold> trustedPolySetToManifold(const PolySet& ps);
 
-  std::shared_ptr<const ManifoldGeometry> createManifoldFromPolySet(const PolySet& ps);
+  std::shared_ptr<ManifoldGeometry> createManifoldFromPolySet(const PolySet& ps);
   std::shared_ptr<const ManifoldGeometry> createManifoldFromGeometry(const std::shared_ptr<const Geometry>& geom);
 
   template <class TriangleMesh>
-  std::shared_ptr<const ManifoldGeometry> createManifoldFromSurfaceMesh(const TriangleMesh& mesh);
+  std::shared_ptr<ManifoldGeometry> createManifoldFromSurfaceMesh(const TriangleMesh& mesh);
 
-  std::shared_ptr<const ManifoldGeometry> applyOperator3DManifold(const Geometry::Geometries& children, OpenSCADOperator op);
+  std::shared_ptr<ManifoldGeometry> 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<const Geometry> applyMinkowskiManifold(const Geometry::Geometries& children);
 #endif
 };