Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Offset: Eliminate Dimples #669

Closed
wants to merge 16 commits into from
Closed

Offset: Eliminate Dimples #669

wants to merge 16 commits into from

Conversation

zalo
Copy link
Contributor

@zalo zalo commented Dec 21, 2023

I had a good idea for the offset implementation while thinking about how OpenCascade does it.

This PR changes how @elalish 's Offset works:

  • Cylinders are replaced with Circular Arcs/Wedges Extruded along each edge.
  • Spheres at Convex Vertices are hulled with all of the circular wedges that end at them.
  • The warped triangles are replaced with a hull of the vertices and their extruded positions (somehow faster in my testing?) Not actually faster, switched back.

The first part fixes the edge dimples, and improves the resolution the offset along convex edges.
The second part fixes the edge dimples that end at the spherical caps.
The third part is just simpler and brought the time down from 1600ms to 1300ms on my test part somehow; not sure why.

The speed of this general process could probably be vastly improved by replacing the spheres with multi-wedge interpolations (map wedges 2D pitch/yaw space, add a bunch more points, delaunay triangulate, lift to 3D again), removing all of the CSG operations, and stitching the faces together by the coincident vertices at the end.

This PR still suffers from the precision issues that Emmett noticed earlier, but they're usually between the extruded triangles on flat faces.

Emmet's PR:
image

This PR (Old, before circularSegments was tuned to match; it does show that edgeResolution and sphereResolution are two independently tunable parameters now):
image

OpenCascade Reference:
image

pca006132 and others added 10 commits December 16, 2023 18:15
* update formatting

* update formatter
* example url

* update

* format

* address comments

* fix formatting
Add WarpBatch - array process warp with single callback
* [python] numpy updates and cleanup

Also add CrossSection::Bounds()

* cleanup - const and reference args in py bind

* fix unnamed args

* all_apis.py more coverage

* fixup - more param renaming, docstring fixes

* fixup - run black

* fixup - move const to left side

* fixup extrude.py get_volume

* fix python examples for api update
Easier than I expected!
@zalo
Copy link
Contributor Author

zalo commented Dec 21, 2023

Another test for fun

Positive 0.1 Offset:
image

Negative 0.1 Offset:
image
image

Kind of cool to see how the circular arcs erode into the shapes; this should enable some fun morphological operations like closing and opening.

Thanks for kicking the idea off, Emmett!

@zalo
Copy link
Contributor Author

zalo commented Dec 21, 2023

Dimple Measurement

Before:
image
image
After:
image
image

It seems like the floater triangles have gotten worse though; let's try switching that part back to the warp based implementation...

EDIT: Yep, switching back to the triangle warping implementation for the triangle extrusion fixed the floaters... so weird. I guess Quickhull is reducing the precision somehow...

@zalo
Copy link
Contributor Author

zalo commented Dec 21, 2023

Hrm, I’ll need to make it so the circular arcs have a variable number of segments based on the angle they subtend.

Maybe specify as “degrees per segment”.

@zalo
Copy link
Contributor Author

zalo commented Dec 21, 2023

Alrighty, I have circularSegments matching the behavior of the original PR; the topology looks just as flawless as the original on edges going in cardinal directions, and the topology isn't quite as clean for tilted edges, but it does completely eliminate the dimpling that arose.

image

I’d expect to get perfect topology at these corners by projecting the wedges to spherical coordinates, doing a dense delaunay triangulation (with poisson samples points in the polygon?), and projecting back to Cartesian coordinates… but this topology should do for now.

Alternatively, the topology looks a lot better when the circularArc resolution is a lot higher than the sphere resolution (as it does in the comparison shots before this comment); not sure what to make of that…

@zalo zalo changed the title Offset: Eliminate Dimples and Improve Resolution Offset: Eliminate Dimples Dec 21, 2023
@zalo
Copy link
Contributor Author

zalo commented Dec 22, 2023

Hmm, a few back and forths with the Offset seem to cause all kinds of havoc, with convex/concave edges sneaking into the mix:

    sphere = manifold3d.Manifold.sphere(0.6, 20)
    cube   = manifold3d.Manifold.cube  (1.0, 1.0, 1.0, True)
    fun_shape = cube - cube.translate(0.5, 0.5, 0.5)

    t0 = perf_counter()
    general_offset = fun_shape     .offset(-0.1, 25)
    general_offset = general_offset.offset( 0.1, 25)
    general_offset = general_offset.offset(-0.1, 25)
    general_offset = general_offset.offset( 0.1, 25)
    save_mesh(general_offset, "elegant_offset", "stl")
    print("Elegant Offset took", (perf_counter()-t0)*1000, "ms")

Elegant Offset took 39279.40459999809 ms
image

Yikes 😅 Let's see how the Minkowski Sum Offset does...

@zalo
Copy link
Contributor Author

zalo commented Dec 22, 2023

Ahhh, it seems like there are more elegant equivalent versions of the morphological opening/closing combo:

    general_offset = fun_shape     .offset(-0.1, 25)
    general_offset = general_offset.offset( 0.2, 25)
    general_offset = general_offset.offset(-0.1, 25)

Elegant Offset took 1735.422899997502 ms
image

    general_offset = fun_shape     .offset( 0.1, 25)
    general_offset = general_offset.offset(-0.2, 25)
    general_offset = general_offset.offset( 0.1, 25)

Elegant Offset took 2998.640499994508 ms
image

@zalo
Copy link
Contributor Author

zalo commented Dec 22, 2023

Minkowski Sum in the same order of dilations/erosions as above:

    small_sphere  : manifold3d.Manifold = manifold3d.Manifold.sphere(0.1, 25)
    small_sphere2 : manifold3d.Manifold = manifold3d.Manifold.sphere(0.2, 25)
    general_offset = fun_shape     .minkowski_difference(small_sphere)
    general_offset = general_offset.minkowski_sum       (small_sphere2)
    general_offset = general_offset.minkowski_difference(small_sphere)
Failed to solve horizon edge.
Minkowski Sum took 3728.7071000027936 ms

image

    small_sphere  : manifold3d.Manifold = manifold3d.Manifold.sphere(0.1, 25)
    small_sphere2 : manifold3d.Manifold = manifold3d.Manifold.sphere(0.2, 25)
    general_offset = fun_shape     .minkowski_sum       (small_sphere)
    general_offset = general_offset.minkowski_difference(small_sphere2)
    general_offset = general_offset.minkowski_sum       (small_sphere)

Minkowski Sum took 2060.8917999998084 ms
image

Seems like you always want to sandwich your erosions between dilations, whether you're using the offset or the minkowski sum 😄

The minkowski sum also doesn't give any floating geometry, which is nice :-)

@zalo
Copy link
Contributor Author

zalo commented Dec 22, 2023

@elalish You might find the above explorations entertaining; it seems like the Minkowski Offsetting method is more robust for layered morphological operations at the moment.

Also worth noting: the Minkowski Offset doesn't experience any degradation when the shape rotates, but the topology of the borders around the interior spherecaps become a little crunchy.

image

(But the surface normals appears to be fine, so it's a method without drawbacks, save for speed.)

@elalish
Copy link
Owner

elalish commented Dec 22, 2023

Yeah, I think you've convinced me on the Minkowski. I'm guessing we can abandon this and #668?

@zalo
Copy link
Contributor Author

zalo commented Dec 22, 2023

Yeah, I think you've convinced me on the Minkowski. I'm guessing we can abandon this and #668?

I’m kind of hopeful that there’s a version of the spherical case that is more robust… iirc it’s over twice as fast as the Minkowski case, and increasing the resolution is a lot cheaper, especially if I can figure out an elegant way to do interpolating spherecaps.

The primary issue for layered morphology is taking care of the corrupting precision issues, which might come for free in the PR that switches everything to doubles? Or maybe allowing vertices to merge after being extruded longer distances somehow?

I suppose the poor man’s vertex merge is quantizing the vertex positions as keys, storing them in an unordered map, and snapping the warp and circular arc positions to them at every step, ensuring that the vertices are coincident before the hulls and booleans.

That, and maybe loosening the tolerance on determining convex edges 🤔

But I probably won’t get to it before the new year; stepping away from my computer for a week starting tomorrow. 🫠

@pca006132
Copy link
Collaborator

I suppose the poor man’s vertex merge is quantizing the vertex positions as keys, storing them in an unordered map, and snapping the warp and circular arc positions to them at every step

OpenSCAD does this for their VBO preview, and the performance is... not very good. openscad/openscad#4883

@pca006132
Copy link
Collaborator

btw I noticed the quickhull module often print Failed to solve horizon edge., wonder if there is a way to fix it or silent it if it is not that important.

https://github.com/akuukka/quickhull/blob/9f81d1ec984a13e3596d02f61c6523e5f2971e82/QuickHull.cpp#L185

@zalo
Copy link
Contributor Author

zalo commented Dec 22, 2023

OpenSCAD does this for their VBO preview, and the performance is... not very good. openscad/openscad#4883

The even poorer man’s version is to just quantize the vertices and not preserve any exactness 🫠
I suspect limiting the vertex precision of morphological operations will come with other unintended stability improvements. Sort of like how layering morphology in pixel/voxel space can be done indefinitely without instability.

btw I noticed the quickhull module often print Failed to solve horizon edge., wonder if there is a way to fix it or silent it if it is not that important.

double all the things 🔥 🤩 🔥
(It’s sad for all of my Hull-based operations, but I don’t think it’s super important if we do end up quantizing vertices.)
Actually, it should either push the vertex to the end of the list to see if it can be solved later, or it should pop vertices from the existing horizon and push them to the end to see if they can be solved later.

I’m just not totally sure if it’s the source of my headaches, since I don’t see it that often 😅

@pca006132
Copy link
Collaborator

I tried implementing the naive minkowski on openscad, and do get them frequently (a few times per model in the benchmarks) iirc. I thought it is already using double? not sure about that.

@zalo
Copy link
Contributor Author

zalo commented Dec 22, 2023

Huh; hadn’t realized OpenSCAD used the same library; I bet it comes from those really crunchy sections where the number of triangles just blows up.

For the particular situation in this thread, I think it’s possible to do the “fillet all edges by radius” operation in one pass without introducing any crunchiness; maybe that’s the 50%-case operation that people actually want from minkowski() and offset()?

I’d have to look at it after Christmas (maybe New Years?) if it is something people want 😅

The key is that Circumcenter code from the Voronoi PR, which will find where to center the exterior circular arcs and spheres to acquire the desired radii 💪

EDIT:
Ah wait, those 2-concave, 1-convex corners are a torus shape; might be a little tricky, especially as the number of edges going into those corners increases. Alright; I’ll let minkowski handle this.

@pca006132
Copy link
Collaborator

no, I mean when they use manifold and manifold calls quickhull. They use CGAL hull in general..

@elalish
Copy link
Owner

elalish commented Dec 23, 2023

To be clear, switching to double will in no way increase robustness or clean up topology. It has exactly the same floating point rounding error problems - the reason to switch is simply to get more precise results (think rivets on a ship).

And quantizing verts to merge them is also a great way to destroy manifoldness - this is exactly why the STL file format can't reliably transmit a manifold. Turns out it just creates even more heinous topology that is nearly impossible to correct in general.

The fact that this special sphere minkowski only gets maybe a 2x speed up tells me its not worth pursuing. If it was 10x, sure, but for the code maintenance burden, I think regular Minkowski is fine. I might make an offset API for convenience though.

@zalo
Copy link
Contributor Author

zalo commented Dec 26, 2023

Thinking about it, there’s probably a market for Offsetting algorithms that don’t increase the number of faces (ala the normal calculation routine in the image here):
#192 (comment)

Calculating average vertex normals first, and then using the Triangle Warp Union on a bunch of extruded vertex triangle faces would handle self-intersections, be fast, and sometimes even be the desired behavior (like when wanting to make a box hollow, preserving sharp edges).

Vertex Normal Extrusion/Dilation is done all the time in video games, but not in a Manifold-preserving way 😄

EDIT:
I could also see it as a new kind of warp function that operates on vertices rather than faces. The vertex transformation can be arbitrary as long as the final union/subtraction/intersection is with coincident triangles…

Probably best to define extrusion amount, resolution (where 0 is a per-vertex extrusion), and Boolean Operation (Union, Subtraction, Intersection)…

* python autogen docstrings

* use function signature hashes in docstring key

* remove sys.path debug code

* docstrings use param names

* autogen docstrings during cmake

* document gen_docs.py

* more readme, remap mesh_gl references

* readme about verify python docstrings, autogen depend on sources
@elalish
Copy link
Owner

elalish commented Dec 26, 2023

Our Warp function does operate on vertices, not faces, and agreed, it's great for lots of things! Pushing verts along their psuedo-normals is certainly a thing you can do, but it's not an offset, and it will often create self-intersections. Probably best left in user space.

@zalo
Copy link
Contributor Author

zalo commented Dec 27, 2023

Ah, I was confused about warp 😅

Just pushed a first-draft (broken) per-vertex-offset implementation:
zalo@721dc5c?w=1
It's worth noting is that it's still creating new per-triangle manifolds and BatchBooleaning them (so there won't be self-intersection issues).

My expectation (as a user) would be that it does something like this when the circularSegments is set to 0 or -1 (as that signifies that no rounding should take place). Ideally, this would output Manifolds with the same number of vertices as the inputs (except for self-intersections).

@elalish
Copy link
Owner

elalish commented Dec 27, 2023

I'm going to go ahead and close this and #668 in favor of #666 for now. I'm not super interested in having a bunch of different offsetting algorithms in Manifold - those can live externally. What would be much more useful if you're up for it, is finding minimal test cases for the bugs we discovered in #666 and #668. Those are important cases to fix for the Boolean, but it's going to incredibly hard for me to debug as it stands. Ideally we want just one Boolean op with just a few faces to repro the problem if possible.

@elalish elalish closed this Dec 27, 2023
@zalo
Copy link
Contributor Author

zalo commented Dec 27, 2023

Alrighty; I'll see if I can make a minimal case. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants