From cdce9e7618f206428548232fb209cda4bf02922a Mon Sep 17 00:00:00 2001 From: Geoff deRosenroll Date: Wed, 22 Mar 2023 21:39:26 -0700 Subject: [PATCH] Improve handling of invalid shape construction. (#385) Add `InvalidConstruction` to `Manifold::Error` and return empty manifolds with it set when manifold construction is attempted with invalid parameters - zero / negative dimensions for Cube - zero / negative radius for Sphere - zero / negative height for Extrude - empty CrossSection for Extrude/Revolve Also, Circle and Square have been updated to return empty CrossSections for similar arguments. --- bindings/c/conv.cpp | 3 +++ bindings/c/include/types.h | 1 + bindings/wasm/bindings.cpp | 3 ++- bindings/wasm/bindings.js | 2 ++ src/cross_section/src/cross_section.cpp | 34 ++++++++++--------------- src/manifold/include/manifold.h | 6 ++--- src/manifold/src/constructors.cpp | 19 +++++++++++++- src/manifold/src/manifold.cpp | 6 +++++ test/cross_section_test.cpp | 7 +++++ test/manifold_test.cpp | 17 +++++++++++++ 10 files changed, 72 insertions(+), 26 deletions(-) diff --git a/bindings/c/conv.cpp b/bindings/c/conv.cpp index e4b685200..db1eb42c3 100644 --- a/bindings/c/conv.cpp +++ b/bindings/c/conv.cpp @@ -96,6 +96,9 @@ ManifoldError to_c(manifold::Manifold::Error error) { case Manifold::Error::FaceIDWrongLength: e = MANIFOLD_FACE_ID_WRONG_LENGTH; break; + case Manifold::Error::InvalidConstruction: + e = MANIFOLD_INVALID_CONSTRUCTION; + break; }; return e; } diff --git a/bindings/c/include/types.h b/bindings/c/include/types.h index 33e809ccf..5268389b1 100644 --- a/bindings/c/include/types.h +++ b/bindings/c/include/types.h @@ -74,6 +74,7 @@ typedef enum ManifoldError { MANIFOLD_TRANSFORM_WRONG_LENGTH, MANIFOLD_RUN_INDEX_WRONG_LENGTH, MANIFOLD_FACE_ID_WRONG_LENGTH, + MANIFOLD_INVALID_CONSTRUCTION, } ManifoldError; typedef enum ManifoldFillRule { diff --git a/bindings/wasm/bindings.cpp b/bindings/wasm/bindings.cpp index 51689a932..8747d8b53 100644 --- a/bindings/wasm/bindings.cpp +++ b/bindings/wasm/bindings.cpp @@ -201,7 +201,8 @@ EMSCRIPTEN_BINDINGS(whatever) { .value("MergeIndexOutOfBounds", Manifold::Error::MergeIndexOutOfBounds) .value("TransformWrongLength", Manifold::Error::TransformWrongLength) .value("RunIndexWrongLength", Manifold::Error::RunIndexWrongLength) - .value("FaceIDWrongLength", Manifold::Error::FaceIDWrongLength); + .value("FaceIDWrongLength", Manifold::Error::FaceIDWrongLength) + .value("InvalidConstruction", Manifold::Error::InvalidConstruction); value_object("box").field("min", &Box::min).field("max", &Box::max); diff --git a/bindings/wasm/bindings.js b/bindings/wasm/bindings.js index 3db660ab5..f29b3b3cc 100644 --- a/bindings/wasm/bindings.js +++ b/bindings/wasm/bindings.js @@ -230,6 +230,8 @@ Module.setup = function() { break; case Module.status.FaceIDWrongLength.value: message = 'Face ID vector has wrong length'; + case Module.status.InvalidConstruction.value: + message = 'Manifold constructed with invalid parameters'; } const base = Error.apply(this, [message, ...args]); diff --git a/src/cross_section/src/cross_section.cpp b/src/cross_section/src/cross_section.cpp index 047e4a3e8..1b63790ac 100644 --- a/src/cross_section/src/cross_section.cpp +++ b/src/cross_section/src/cross_section.cpp @@ -14,20 +14,6 @@ #include "cross_section.h" -#include - -#include -#include - -#include "clipper2/clipper.core.h" -#include "clipper2/clipper.engine.h" -#include "clipper2/clipper.offset.h" -#include "glm/ext/matrix_float3x2.hpp" -#include "glm/ext/vector_float2.hpp" -#include "glm/geometric.hpp" -#include "glm/glm.hpp" -#include "public.h" - using namespace manifold; namespace { @@ -230,23 +216,28 @@ C2::PathsD CrossSection::GetPaths() const { /** * Constructs a square with the given XY dimensions. By default it is - * positioned in the first quadrant, touching the origin. + * positioned in the first quadrant, touching the origin. If any dimensions in + * size are negative, or if all are zero, an empty Manifold will be returned. * * @param size The X, and Y dimensions of the square. * @param center Set to true to shift the center to the origin. */ -CrossSection CrossSection::Square(const glm::vec2 dims, bool center) { +CrossSection CrossSection::Square(const glm::vec2 size, bool center) { + if (size.x < 0.0f || size.y < 0.0f || glm::length(size) == 0.0f) { + return CrossSection(); + } + auto p = C2::PathD(4); if (center) { - auto w = dims.x / 2; - auto h = dims.y / 2; + const auto w = size.x / 2; + const auto h = size.y / 2; p[0] = C2::PointD(w, h); p[1] = C2::PointD(-w, h); p[2] = C2::PointD(-w, -h); p[3] = C2::PointD(w, -h); } else { - double x = dims.x; - double y = dims.y; + const double x = size.x; + const double y = size.y; p[0] = C2::PointD(0.0, 0.0); p[1] = C2::PointD(x, 0.0); p[2] = C2::PointD(x, y); @@ -263,6 +254,9 @@ CrossSection CrossSection::Square(const glm::vec2 dims, bool center) { * calculated by the static Quality defaults according to the radius. */ CrossSection CrossSection::Circle(float radius, int circularSegments) { + if (radius <= 0.0f) { + return CrossSection(); + } int n = circularSegments > 2 ? circularSegments : Quality::GetCircularSegments(radius); float dPhi = 360.0f / n; diff --git a/src/manifold/include/manifold.h b/src/manifold/include/manifold.h index 42e611b65..73fa48723 100644 --- a/src/manifold/include/manifold.h +++ b/src/manifold/include/manifold.h @@ -97,6 +97,7 @@ class Manifold { TransformWrongLength, RunIndexWrongLength, FaceIDWrongLength, + InvalidConstruction, }; Error Status() const; int NumVert() const; @@ -171,14 +172,11 @@ class Manifold { private: Manifold(std::shared_ptr pNode_); Manifold(std::shared_ptr pImpl_); + static Manifold Invalid(); mutable std::shared_ptr pNode_; CsgLeafNode& GetCsgLeafNode() const; - - static int circularSegments_; - static float circularAngle_; - static float circularEdgeLength_; }; /** @} */ } // namespace manifold diff --git a/src/manifold/src/constructors.cpp b/src/manifold/src/constructors.cpp index 0ad740141..9243bdef6 100644 --- a/src/manifold/src/constructors.cpp +++ b/src/manifold/src/constructors.cpp @@ -143,12 +143,17 @@ Manifold Manifold::Tetrahedron() { /** * Constructs a unit cube (edge lengths all one), by default in the first - * octant, touching the origin. + * octant, touching the origin. If any dimensions in size are negative, or if + * all are zero, an empty Manifold will be returned. * * @param size The X, Y, and Z dimensions of the box. * @param center Set to true to shift the center to the origin. */ Manifold Manifold::Cube(glm::vec3 size, bool center) { + if (size.x < 0.0f || size.y < 0.0f || size.z < 0.0f || + glm::length(size) == 0.) { + return Invalid(); + } auto cube = Manifold(std::make_shared(Impl::Shape::Cube)); cube = cube.Scale(size); if (center) cube = cube.Translate(-size / 2.0f); @@ -170,6 +175,9 @@ Manifold Manifold::Cube(glm::vec3 size, bool center) { */ Manifold Manifold::Cylinder(float height, float radiusLow, float radiusHigh, int circularSegments, bool center) { + if (height <= 0.0f || radiusLow <= 0.0f) { + return Invalid(); + } float scale = radiusHigh >= 0.0f ? radiusHigh / radiusLow : 1.0f; float radius = fmax(radiusLow, radiusHigh); int n = circularSegments > 2 ? circularSegments @@ -195,6 +203,9 @@ Manifold Manifold::Cylinder(float height, float radiusLow, float radiusHigh, * calculated by the static Defaults. */ Manifold Manifold::Sphere(float radius, int circularSegments) { + if (radius <= 0.0f) { + return Invalid(); + } int n = circularSegments > 0 ? (circularSegments + 3) / 4 : Quality::GetCircularSegments(radius) / 4; auto pImpl_ = std::make_shared(Impl::Shape::Octahedron); @@ -226,6 +237,9 @@ Manifold Manifold::Extrude(const CrossSection& crossSection, float height, int nDivisions, float twistDegrees, glm::vec2 scaleTop) { auto polygons = crossSection.ToPolygons(); + if (polygons.size() == 0 || height <= 0.0f) { + return Invalid(); + } scaleTop.x = glm::max(scaleTop.x, 0.0f); scaleTop.y = glm::max(scaleTop.y, 0.0f); @@ -307,6 +321,9 @@ Manifold Manifold::Extrude(const CrossSection& crossSection, float height, Manifold Manifold::Revolve(const CrossSection& crossSection, int circularSegments) { auto polygons = crossSection.ToPolygons(); + if (polygons.size() == 0) { + return Invalid(); + } float radius = 0.0f; for (const auto& poly : polygons) { diff --git a/src/manifold/src/manifold.cpp b/src/manifold/src/manifold.cpp index 6461cae09..162163e1c 100644 --- a/src/manifold/src/manifold.cpp +++ b/src/manifold/src/manifold.cpp @@ -71,6 +71,12 @@ Manifold::Manifold(std::shared_ptr pNode) : pNode_(pNode) {} Manifold::Manifold(std::shared_ptr pImpl_) : pNode_(std::make_shared(pImpl_)) {} +Manifold Manifold::Invalid() { + auto pImpl_ = std::make_shared(); + pImpl_->status_ = Error::InvalidConstruction; + return Manifold(pImpl_); +} + Manifold& Manifold::operator=(const Manifold& other) { if (this != &other) { pNode_ = other.pNode_; diff --git a/test/cross_section_test.cpp b/test/cross_section_test.cpp index 640e627d5..2ac4260b9 100644 --- a/test/cross_section_test.cpp +++ b/test/cross_section_test.cpp @@ -28,6 +28,13 @@ using namespace manifold; +TEST(CrossSection, Square) { + auto a = Manifold::Cube({5, 5, 5}); + auto b = Manifold::Extrude(CrossSection::Square({5, 5}), 5); + + EXPECT_FLOAT_EQ((a - b).GetProperties().volume, 0.); +} + TEST(CrossSection, MirrorUnion) { auto a = CrossSection::Square({5., 5.}, true); auto b = a.Translate({2.5, 2.5}); diff --git a/test/manifold_test.cpp b/test/manifold_test.cpp index 52a8135bb..ff5af69b0 100644 --- a/test/manifold_test.cpp +++ b/test/manifold_test.cpp @@ -14,6 +14,7 @@ #include "manifold.h" +#include "cross_section.h" #include "test.h" #ifdef MANIFOLD_EXPORT @@ -505,3 +506,19 @@ TEST(Manifold, MirrorUnion) { EXPECT_FLOAT_EQ(vol_a * 2.75, result.GetProperties().volume); EXPECT_TRUE(a.Mirror(glm::vec3(0)).IsEmpty()); } + +TEST(Manifold, Invalid) { + auto invalid = Manifold::Error::InvalidConstruction; + auto circ = CrossSection::Circle(10.); + auto empty_circ = CrossSection::Circle(-2.); + auto empty_sq = CrossSection::Square(glm::vec3(0.0f)); + + EXPECT_EQ(Manifold::Sphere(0).Status(), invalid); + EXPECT_EQ(Manifold::Cylinder(0, 5).Status(), invalid); + EXPECT_EQ(Manifold::Cylinder(2, -5).Status(), invalid); + EXPECT_EQ(Manifold::Cube(glm::vec3(0.0f)).Status(), invalid); + EXPECT_EQ(Manifold::Cube({-1, 1, 1}).Status(), invalid); + EXPECT_EQ(Manifold::Extrude(circ, 0.).Status(), invalid); + EXPECT_EQ(Manifold::Extrude(empty_circ, 10.).Status(), invalid); + EXPECT_EQ(Manifold::Revolve(empty_sq).Status(), invalid); +}