From 8f8ccafb71f01d33e08a19b1270bdb11523ba6c3 Mon Sep 17 00:00:00 2001 From: Geoff deRosenroll Date: Mon, 20 Mar 2023 12:42:56 -0700 Subject: [PATCH 1/9] Add InvalidConstruction to Manifold::Error --- bindings/c/conv.cpp | 3 +++ bindings/c/include/types.h | 1 + src/manifold/include/manifold.h | 1 + 3 files changed, 5 insertions(+) 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/src/manifold/include/manifold.h b/src/manifold/include/manifold.h index 42e611b65..9eab2c24d 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; From 79cf6c31ab9d2c1dea3ea48457a2b0b7eddfa619 Mon Sep 17 00:00:00 2001 From: Geoff deRosenroll Date: Mon, 20 Mar 2023 13:07:11 -0700 Subject: [PATCH 2/9] Add Invalid private constructor with use it --- src/manifold/include/manifold.h | 5 +---- src/manifold/src/constructors.cpp | 15 +++++++++++++++ src/manifold/src/manifold.cpp | 6 ++++++ 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/manifold/include/manifold.h b/src/manifold/include/manifold.h index 9eab2c24d..73fa48723 100644 --- a/src/manifold/include/manifold.h +++ b/src/manifold/include/manifold.h @@ -172,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..90b5ea1b4 100644 --- a/src/manifold/src/constructors.cpp +++ b/src/manifold/src/constructors.cpp @@ -149,6 +149,9 @@ Manifold Manifold::Tetrahedron() { * @param center Set to true to shift the center to the origin. */ Manifold Manifold::Cube(glm::vec3 size, bool center) { + if (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 +173,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 || radiusHigh < 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 +201,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 +235,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 +319,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_; From 6c05d07bd0b84c1a18e58e0f09021e34a7c6b098 Mon Sep 17 00:00:00 2001 From: Geoff deRosenroll Date: Tue, 21 Mar 2023 11:43:42 -0700 Subject: [PATCH 3/9] CrossSection validity checks + neg Square winding --- src/cross_section/src/cross_section.cpp | 33 +++++++++++++++++++------ 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/src/cross_section/src/cross_section.cpp b/src/cross_section/src/cross_section.cpp index 047e4a3e8..f7b1a0643 100644 --- a/src/cross_section/src/cross_section.cpp +++ b/src/cross_section/src/cross_section.cpp @@ -236,21 +236,35 @@ C2::PathsD CrossSection::GetPaths() const { * @param center Set to true to shift the center to the origin. */ CrossSection CrossSection::Square(const glm::vec2 dims, bool center) { + if (glm::length(dims) == 0.0f) { + return CrossSection(); + } + + // reverse winding if dimensions are negative + int start, step; + if (dims.x * dims.y < 0.0f) { + start = 3; + step = -1; + } else { + start = 0; + step = 1; + } + auto p = C2::PathD(4); if (center) { auto w = dims.x / 2; auto h = dims.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); + p[start + step * 0] = C2::PointD(w, h); + p[start + step * 1] = C2::PointD(-w, h); + p[start + step * 2] = C2::PointD(-w, -h); + p[start + step * 3] = C2::PointD(w, -h); } else { double x = dims.x; double y = dims.y; - p[0] = C2::PointD(0.0, 0.0); - p[1] = C2::PointD(x, 0.0); - p[2] = C2::PointD(x, y); - p[3] = C2::PointD(0.0, y); + p[start + step * 0] = C2::PointD(0.0, 0.0); + p[start + step * 1] = C2::PointD(x, 0.0); + p[start + step * 2] = C2::PointD(x, y); + p[start + step * 3] = C2::PointD(0.0, y); } return CrossSection(C2::PathsD{p}); } @@ -263,6 +277,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; From dc8fa8994fe122b21665878d7c400f62a91c918a Mon Sep 17 00:00:00 2001 From: Geoff deRosenroll Date: Tue, 21 Mar 2023 11:44:39 -0700 Subject: [PATCH 4/9] Remove radiusHigh check (incorrect handling) --- src/manifold/src/constructors.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/manifold/src/constructors.cpp b/src/manifold/src/constructors.cpp index 90b5ea1b4..ff120a16e 100644 --- a/src/manifold/src/constructors.cpp +++ b/src/manifold/src/constructors.cpp @@ -173,7 +173,7 @@ 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 || radiusHigh < 0.0f) { + if (height <= 0.0f || radiusLow <= 0.0f) { return Invalid(); } float scale = radiusHigh >= 0.0f ? radiusHigh / radiusLow : 1.0f; From 4d2755aff07f7da75763cd169481f4e513c517e6 Mon Sep 17 00:00:00 2001 From: Geoff deRosenroll Date: Tue, 21 Mar 2023 11:46:07 -0700 Subject: [PATCH 5/9] Tests for invalid construction and neg squares --- test/cross_section_test.cpp | 18 ++++++++++++++++++ test/manifold_test.cpp | 16 ++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/test/cross_section_test.cpp b/test/cross_section_test.cpp index 640e627d5..14408ba2d 100644 --- a/test/cross_section_test.cpp +++ b/test/cross_section_test.cpp @@ -28,6 +28,24 @@ using namespace manifold; +TEST(CrossSection, Square) { + auto sq = CrossSection::Square({5, 5}); + auto neg_x_sq = CrossSection::Square({-5, 5}); + auto neg_y_sq = CrossSection::Square({5, -5}); + auto neg_xy_sq = CrossSection::Square({-5, -5}); + + auto cb = Manifold::Cube({5, 5, 5}); + auto neg_x_cb = Manifold::Cube({-5, 5, 5}); + auto neg_y_cb = Manifold::Cube({5, -5, 5}); + auto neg_xy_cb = Manifold::Cube({-5, -5, 5}); + + auto cubes = cb + neg_x_cb + neg_y_cb + neg_xy_cb; + auto ex = Manifold::Extrude(sq + neg_x_sq + neg_y_sq + neg_xy_sq, 5); + + // ensure semantics for negative dimensions for square and cube agree + EXPECT_FLOAT_EQ((cubes - ex).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..440e1f7f4 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,18 @@ 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::Extrude(circ, 0.).Status(), invalid); + EXPECT_EQ(Manifold::Extrude(empty_circ, 10.).Status(), invalid); + EXPECT_EQ(Manifold::Revolve(empty_sq).Status(), invalid); +} From d854db4d117c730fdd10f6619c812c2902e5c539 Mon Sep 17 00:00:00 2001 From: Geoff deRosenroll Date: Tue, 21 Mar 2023 13:48:00 -0700 Subject: [PATCH 6/9] Use const in CrossSection::Square --- src/cross_section/src/cross_section.cpp | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/cross_section/src/cross_section.cpp b/src/cross_section/src/cross_section.cpp index f7b1a0643..860a9840c 100644 --- a/src/cross_section/src/cross_section.cpp +++ b/src/cross_section/src/cross_section.cpp @@ -17,6 +17,7 @@ #include #include +#include #include #include "clipper2/clipper.core.h" @@ -241,26 +242,21 @@ CrossSection CrossSection::Square(const glm::vec2 dims, bool center) { } // reverse winding if dimensions are negative - int start, step; - if (dims.x * dims.y < 0.0f) { - start = 3; - step = -1; - } else { - start = 0; - step = 1; - } + const bool neg = dims.x * dims.y < 0.0f; + const int start = neg ? 3 : 0; + const int step = neg ? -1 : 1; auto p = C2::PathD(4); if (center) { - auto w = dims.x / 2; - auto h = dims.y / 2; + const auto w = dims.x / 2; + const auto h = dims.y / 2; p[start + step * 0] = C2::PointD(w, h); p[start + step * 1] = C2::PointD(-w, h); p[start + step * 2] = C2::PointD(-w, -h); p[start + step * 3] = C2::PointD(w, -h); } else { - double x = dims.x; - double y = dims.y; + const double x = dims.x; + const double y = dims.y; p[start + step * 0] = C2::PointD(0.0, 0.0); p[start + step * 1] = C2::PointD(x, 0.0); p[start + step * 2] = C2::PointD(x, y); From f4e24a69241d0256c16acd51817f9af3628753af Mon Sep 17 00:00:00 2001 From: Geoff deRosenroll Date: Tue, 21 Mar 2023 23:57:39 -0700 Subject: [PATCH 7/9] Negative dimensions now invalid for Square/Cube --- src/cross_section/src/cross_section.cpp | 36 +++++++++++-------------- src/manifold/src/constructors.cpp | 6 +++-- test/cross_section_test.cpp | 17 +++--------- test/manifold_test.cpp | 1 + 4 files changed, 24 insertions(+), 36 deletions(-) diff --git a/src/cross_section/src/cross_section.cpp b/src/cross_section/src/cross_section.cpp index 860a9840c..1bfd66387 100644 --- a/src/cross_section/src/cross_section.cpp +++ b/src/cross_section/src/cross_section.cpp @@ -231,36 +231,32 @@ 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) { - if (glm::length(dims) == 0.0f) { +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(); } - // reverse winding if dimensions are negative - const bool neg = dims.x * dims.y < 0.0f; - const int start = neg ? 3 : 0; - const int step = neg ? -1 : 1; - auto p = C2::PathD(4); if (center) { - const auto w = dims.x / 2; - const auto h = dims.y / 2; - p[start + step * 0] = C2::PointD(w, h); - p[start + step * 1] = C2::PointD(-w, h); - p[start + step * 2] = C2::PointD(-w, -h); - p[start + step * 3] = C2::PointD(w, -h); + 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 { - const double x = dims.x; - const double y = dims.y; - p[start + step * 0] = C2::PointD(0.0, 0.0); - p[start + step * 1] = C2::PointD(x, 0.0); - p[start + step * 2] = C2::PointD(x, y); - p[start + step * 3] = C2::PointD(0.0, 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); + p[3] = C2::PointD(0.0, y); } return CrossSection(C2::PathsD{p}); } diff --git a/src/manifold/src/constructors.cpp b/src/manifold/src/constructors.cpp index ff120a16e..9243bdef6 100644 --- a/src/manifold/src/constructors.cpp +++ b/src/manifold/src/constructors.cpp @@ -143,13 +143,15 @@ 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 (glm::length(size) == 0.) { + 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)); diff --git a/test/cross_section_test.cpp b/test/cross_section_test.cpp index 14408ba2d..2ac4260b9 100644 --- a/test/cross_section_test.cpp +++ b/test/cross_section_test.cpp @@ -29,21 +29,10 @@ using namespace manifold; TEST(CrossSection, Square) { - auto sq = CrossSection::Square({5, 5}); - auto neg_x_sq = CrossSection::Square({-5, 5}); - auto neg_y_sq = CrossSection::Square({5, -5}); - auto neg_xy_sq = CrossSection::Square({-5, -5}); + auto a = Manifold::Cube({5, 5, 5}); + auto b = Manifold::Extrude(CrossSection::Square({5, 5}), 5); - auto cb = Manifold::Cube({5, 5, 5}); - auto neg_x_cb = Manifold::Cube({-5, 5, 5}); - auto neg_y_cb = Manifold::Cube({5, -5, 5}); - auto neg_xy_cb = Manifold::Cube({-5, -5, 5}); - - auto cubes = cb + neg_x_cb + neg_y_cb + neg_xy_cb; - auto ex = Manifold::Extrude(sq + neg_x_sq + neg_y_sq + neg_xy_sq, 5); - - // ensure semantics for negative dimensions for square and cube agree - EXPECT_FLOAT_EQ((cubes - ex).GetProperties().volume, 0.); + EXPECT_FLOAT_EQ((a - b).GetProperties().volume, 0.); } TEST(CrossSection, MirrorUnion) { diff --git a/test/manifold_test.cpp b/test/manifold_test.cpp index 440e1f7f4..ff5af69b0 100644 --- a/test/manifold_test.cpp +++ b/test/manifold_test.cpp @@ -517,6 +517,7 @@ TEST(Manifold, 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); From edecda2e45e0b50f2a84b30be5911e49efaa50b6 Mon Sep 17 00:00:00 2001 From: Geoff deRosenroll Date: Wed, 22 Mar 2023 12:43:06 -0700 Subject: [PATCH 8/9] Remove includes already in header --- src/cross_section/src/cross_section.cpp | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/src/cross_section/src/cross_section.cpp b/src/cross_section/src/cross_section.cpp index 1bfd66387..1b63790ac 100644 --- a/src/cross_section/src/cross_section.cpp +++ b/src/cross_section/src/cross_section.cpp @@ -14,21 +14,6 @@ #include "cross_section.h" -#include - -#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 { From 0f24d0d9fb6882c4ab99b54bc91a1a1ab21bd985 Mon Sep 17 00:00:00 2001 From: Geoff deRosenroll Date: Wed, 22 Mar 2023 12:50:19 -0700 Subject: [PATCH 9/9] Add InvalidConstruction to wasm bindings --- bindings/wasm/bindings.cpp | 3 ++- bindings/wasm/bindings.js | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) 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]);