Partial Revolve #455

cartesian-theatrics · 2023-06-17T03:20:51Z

This adds an optional arity for Revolve that supports partial revolutions. It changes the implementation slightly by first taking the x>=0 slice if the bounding box contains negative x values. The cases of vertices lying on the revolve axis is handled in the same loop, arguably simplifying the logic slightly and making it easier to support partial evolves. I believe the tessellation of full revolutions is equivalent to the previous version.

It might be worth noting it's not strictly required to implement special handling of coincident vertices. The logic is far simpler in this case and the core implementation seems to be able handle it no problem. However, it does break Genus. I think it's better to do the correct construction, but might be something to consider. The complexity overhead is quite high unfortunately... Perhaps it can be simplified.

google-cla · 2023-06-17T03:20:54Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

elalish

Thanks, this looks promising. Any trouble signing the CLA?

elalish · 2023-06-18T04:11:54Z

test/manifold_test.cpp

+TEST(Manifold, PartialRevolve) {
+  Polygons polys = SquareHole(2.0f);
+  Manifold donutHole = Manifold::Revolve(polys, 48, 180);
+  EXPECT_EQ(donutHole.Genus(), 1);


Would you mind pasting an image of this object? I'm having trouble visualizing what axis it's rotating around.

Yes, it looks like this. I'd definitely want to do more thorough testing before merging (even if we don't want to include them all).

Thanks; I'd recommend pushing it either in or out a little, since right now it's just rounding error determining a major topological change in the center.

elalish · 2023-06-18T04:13:08Z

src/manifold/src/constructors.cpp

+          if (currPolyVertex.x > 0.0) {
+            triVerts.push_back({startPosIndex + slice,
+                startPosIndex + lastSlice,
+                // "Reuse" vertex of first slice if it lies on the revolve axis


Is this the part you were saying messes up the genus? Can you give an example?

Kinda. Genus "breaks" when you don't have any special handling of vertices lying on the Y axis. When x=0, {pos.x * manifold::cosd(phi), pos.x * manifold::sind(phi), pos.y} yields coincident vertices and degenerate triangles. For example, when a square is offset from the Y axis Revolve will yield 8 triangles and 4 vertices per slice, whereas if one edge is on the Y axis, Revolve will yield 4 triangles and two vertices per slice. In my initial implementation I didn't have any special handling of such cases but surprisingly it still mostly worked. But Genus was off I guess because Manifold considered the revolved square to be an infinitesimal toroid.

This is the original loop that "works":

for (const auto& poly : polygons) { for (std::size_t polyVert = 0; polyVert < poly.size(); ++polyVert) { int startVert = vertPos.size(); if (!isFullRevolution) startPoses.push_back(startVert); // first and last slice are distinguished if not a full revolution. int nSlices = isFullRevolution ? nDivisions : nDivisions + 1; int lastStart = startVert + (polyVert == 0 ? nSlices * (poly.size() - 1) : -nSlices); for (int slice = 0; slice < nSlices; ++slice) { float phi = slice * dPhi; glm::vec2 pos = poly[polyVert]; glm::vec3 p = {pos.x * manifold::cosd(phi), pos.x * manifold::sind(phi), pos.y}; vertPos.push_back(p); int lastSlice = (slice == 0 ? nDivisions : slice) - 1; if (isFullRevolution || slice > 0) { triVerts.push_back({startVert + slice, startVert + lastSlice, lastStart + lastSlice}); triVerts.push_back( {lastStart + lastSlice, lastStart + slice, startVert + slice}); } } if (!isFullRevolution) endPoses.push_back(vertPos.size() -1); } }

Right, so the problem is without special handling of the center verts you end up with an infinitesimal hole through it like a bead. Definitely important to avoid that, since I don't think our decimator can fix it. If your test case doesn't already check that, it's probably a good idea to add a test for that specifically.

elalish · 2023-06-18T04:13:39Z

src/manifold/src/constructors.cpp

+    CrossSection posBoundingBox = CrossSection({{0.0, min.y},{max.x, min.y},
+                                                {max.x,max.y},{0.0,max.y}});
+
+    // Can't use RectClip as it has many failure cases.


That seems odd - what failure cases have you seen?

I wish I would have kept failing cases. I will try to recreate them.

Even in the plain Revolve test case this doesn't seem to work.

if (bounds.min.x < 0) { glm::vec2 min = bounds.min; glm::vec2 max = bounds.max; Rect boundingBox = Rect({0.0, min.y}, {max.x, max.y}); // Can't use RectClip as it has many failure cases. polygons = crossSection.RectClip(boundingBox).ToPolygons(); }

Result is not manifold-2, doesn't export. Am I constructing the Rect wrong?

I'm not seeing a manifoldness failure, but I'm definitely reproducing a serious problem with RectClip: When I clip a polygon with a hole in half, I'm only getting the outer contour; intersection gives the correct result. @geoffder would you mind taking a look?

This is fine for this PR, thanks!

elalish · 2023-06-18T04:15:30Z

src/manifold/src/constructors.cpp

+  std::vector<int> startPoses;
+  std::vector<int> endPoses;
+
+  float dPhi = revolveDegrees / nDivisions;


I've been trying to update our style to use const as much as possible; would you mind applying that across your PR?

Yep, good call.

elalish · 2023-07-03T15:55:26Z

@cartesian-theatrics Is anything blocking you on this PR? I would like to merge it.

cartesian-theatrics · 2023-07-03T22:09:04Z

My apologies, got distracted with other priorities. I will work this today.

codecov · 2023-07-04T01:51:36Z

Codecov Report

Patch coverage: 95.65% and project coverage change: +0.15 🎉

Comparison is base (dd3445d) 89.94% compared to head (4eff58f) 90.09%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #455      +/-   ##
==========================================
+ Coverage   89.94%   90.09%   +0.15%     
==========================================
  Files          35       35              
  Lines        4286     4291       +5     
==========================================
+ Hits         3855     3866      +11     
+ Misses        431      425       -6

Impacted Files	Coverage Δ
src/manifold/include/manifold.h	`100.00% <ø> (ø)`
src/manifold/src/constructors.cpp	`94.79% <95.65%> (-0.40%)`	⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

elalish · 2023-07-04T05:24:52Z

Thanks! Do whatever's easiest with the CLA - if you want to make a different branch with a new PR, that's fine. Let us know when you're ready for review.

cartesian-theatrics · 2023-07-04T16:11:54Z

@elalish other than the unresolved RectClip issue (see my comment), I think this is ready for review.

elalish

This is looking great, just a few minor things. Thanks for cleaning up my code!

elalish · 2023-07-04T15:44:51Z

bindings/wasm/manifold-encapsulated-types.d.ts

-      Manifold;
+  static revolve(
+      polygons: CrossSection|Polygons, circularSegments?: number,
+      revolveDegrees?: number): Manifold;


Do we also need to update CrossSection.revolve?

elalish · 2023-07-04T15:50:41Z

test/manifold_test.cpp

+                  rotatedPolygon.end());
+      rotatedPolys.push_back(rotatedPolygon);
+    }
+    Manifold vug = Manifold::Revolve(rotatedPolys, 48);


Thanks for expanding this test case - I think you can remove the unused vug at the top now.

elalish · 2023-07-04T15:52:54Z

test/manifold_test.cpp

+      std::rotate(rotatedPolygon.begin(), rotatedPolygon.begin() + i,
+                  rotatedPolygon.end());
+      rotatedOffsetPolys.push_back(rotatedPolygon);
+    }


Given the reuse, perhaps this could be a test helper function?

elalish · 2023-07-04T15:55:19Z

test/manifold_test.cpp

@@ -14,6 +14,8 @@

 #include "manifold.h"

+#include <iostream>


elalish · 2023-07-04T15:57:27Z

test/manifold_test.cpp

@@ -215,6 +227,44 @@ TEST(Manifold, Revolve2) {
  EXPECT_NEAR(prop.surfaceArea, 96.0f * glm::pi<float>(), 1.0f);
 }

+TEST(Manifold, PartialRevolve) {
+  Polygons polys = SquareHole(2.0f);
+  Polygons offsetPolys = SquareHole(10.0f);


Since these appear to be independent, how about splitting them into two separate test cases?

elalish · 2023-07-04T16:22:05Z

src/manifold/src/constructors.cpp

+    CrossSection posBoundingBox = CrossSection({{0.0, min.y},{max.x, min.y},
+                                                {max.x,max.y},{0.0,max.y}});
+
+    // Can't use RectClip as it has many failure cases.


I'm not seeing a manifoldness failure, but I'm definitely reproducing a serious problem with RectClip: When I clip a polygon with a hole in half, I'm only getting the outer contour; intersection gives the correct result. @geoffder would you mind taking a look?

This is fine for this PR, thanks!

geoffder · 2023-07-04T16:44:44Z

This sounded familiar so I went to the Clipper2 issue tracker and it may be related to a known limitation mentioned in January AngusJohnson/Clipper2#386.

edit: Had the feeling is see more of this story before, and there was followup discussion: AngusJohnson/Clipper2#383. So the multiple polygon results was fixed I think, but in his comments here he does mention that RectClip is fill rule agnostic as it was then (and still now based on your findings) so it doesn't know the difference between holes and outlines.

elalish · 2023-07-04T22:36:27Z

Thanks, @geoffder, we can follow up on those Clipper2 threads. The behavior I saw was worse than the examples he showed, but probably the same issue. If we can't get a proper fill-rule preserving RectClip, I think I'd prefer to remove it from our API, due to this kind of confusion.

elalish

Thanks!

geoffder · 2023-07-05T00:43:12Z

Thanks, @geoffder, we can follow up on those Clipper2 threads. The behavior I saw was worse than the examples he showed, but probably the same issue. If we can't get a proper fill-rule preserving RectClip, I think I'd prefer to remove it from our API, due to this kind of confusion.

Hmm I just tested clipping a ring (circle with opposite winding circular hole) and got the same result with intersection and rect clip in Clipper2 (via my ocaml-clipper2 bindings), using their svg export. But, there is definitely funny business when it comes to trying to use the output in manifold.

  auto rect = Rect({0, 0}, {5, 5});
  auto ring = CrossSection::Circle(4) - CrossSection::Circle(2);
  auto clipped_ring = ring.RectClip(rect);
  auto clipped_ring_ex = Manifold::Extrude(ring.RectClip(rect), 5); // empty result
  auto inter_ring_ex = Manifold::Extrude(ring ^ rect.AsCrossSection(), 5);  // works after AsCrossSection fix
  EXPECT_EQ(clipped_ring.NumContour(), 1); // fails, with 2

Going by Angus' comments in the linked discussions, this is because RectClip simply tracks each contour and clips them independently, while leaving the winding order alone. I would have thought that going through a union would be fine if the winding was left intact though (if you add an empty CrossSection to the clipped_ring the result is odd). Since the usual clipping algorithms aren't applied, the usual guarantees that we have for our CrossSections are out the window I think. So, maybe leaving RectClip out for now would be for the best.

While testing this, I've found a bug (my fault) with the winding of the path made in AsCrossSection that results in an empty shape after the Clipper2 union, so I'll make a PR to fix that along with dropping RectClip from our API later.

Following discussion on #455, this removes RectClip from our API, as it does not provide the same guarantees that Clipper2's regular boolean clipping operations give us (that make sure that CrossSections can be extruded successfully). Also, this fixes AsCrossSection which was returning empty CrossSections due to incorrect winding followed by a Positive fill rule union operation. Now it is working as advertised, while skipping over the needless clipping by implementing it as a CrossSection constructor (building a PathsD directly).

* handle verts on revolve axis * Tests working * Const declares, test cycle start vert * clang-format * clang-format test code * fix header declaration * WASM bindings * format bindings.js * clang format * Address comments

Following discussion on elalish#455, this removes RectClip from our API, as it does not provide the same guarantees that Clipper2's regular boolean clipping operations give us (that make sure that CrossSections can be extruded successfully). Also, this fixes AsCrossSection which was returning empty CrossSections due to incorrect winding followed by a Positive fill rule union operation. Now it is working as advertised, while skipping over the needless clipping by implementing it as a CrossSection constructor (building a PathsD directly).

elalish reviewed Jun 18, 2023

View reviewed changes

jmicahc force-pushed the partial-revolution branch 2 times, most recently from 327ccbc to e59a9d1 Compare July 4, 2023 07:20

cartesian-theatrics added 9 commits July 4, 2023 00:41

handle verts on revolve axis

231088c

Tests working

2980f76

Const declares, test cycle start vert

9bbde90

clang-format

c3973ff

clang-format test code

81d21f2

fix header declaration

18dd8b4

WASM bindings

94049ff

format bindings.js

ea858c5

clang format

0ad0b10

jmicahc force-pushed the partial-revolution branch from 4c0bbc6 to 0ad0b10 Compare July 4, 2023 07:43

elalish requested changes Jul 4, 2023

View reviewed changes

Address comments

4eff58f

elalish approved these changes Jul 4, 2023

View reviewed changes

elalish merged commit 6cec951 into elalish:master Jul 4, 2023

geoffder mentioned this pull request Jul 5, 2023

Remove RectClip API + Rect::AsCrossSection fix #478

Merged

elalish mentioned this pull request Nov 3, 2023

V2.2.1 #589

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partial Revolve #455

Partial Revolve #455

cartesian-theatrics commented Jun 17, 2023

google-cla bot commented Jun 17, 2023

elalish left a comment

elalish Jun 18, 2023

cartesian-theatrics Jun 18, 2023

elalish Jun 19, 2023

elalish Jun 18, 2023

cartesian-theatrics Jun 18, 2023

elalish Jun 18, 2023

elalish Jun 18, 2023

cartesian-theatrics Jun 18, 2023

cartesian-theatrics Jul 4, 2023

elalish Jul 4, 2023

elalish Jun 18, 2023

cartesian-theatrics Jun 18, 2023

elalish commented Jul 3, 2023

cartesian-theatrics commented Jul 3, 2023

codecov bot commented Jul 4, 2023 •

edited

Loading

elalish commented Jul 4, 2023

cartesian-theatrics commented Jul 4, 2023

elalish left a comment

elalish Jul 4, 2023

elalish Jul 4, 2023

elalish Jul 4, 2023

elalish Jul 4, 2023

elalish Jul 4, 2023

elalish Jul 4, 2023

geoffder commented Jul 4, 2023 •

edited

Loading

elalish commented Jul 4, 2023

elalish left a comment

geoffder commented Jul 5, 2023

Partial Revolve #455

Partial Revolve #455

Conversation

cartesian-theatrics commented Jun 17, 2023

google-cla bot commented Jun 17, 2023

elalish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elalish commented Jul 3, 2023

cartesian-theatrics commented Jul 3, 2023

codecov bot commented Jul 4, 2023 • edited Loading

Codecov Report

elalish commented Jul 4, 2023

cartesian-theatrics commented Jul 4, 2023

elalish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoffder commented Jul 4, 2023 • edited Loading

elalish commented Jul 4, 2023

elalish left a comment

Choose a reason for hiding this comment

geoffder commented Jul 5, 2023

codecov bot commented Jul 4, 2023 •

edited

Loading

geoffder commented Jul 4, 2023 •

edited

Loading