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

Mesh simplification after warp is often undesirable #673

Closed
wrongbad opened this issue Dec 26, 2023 · 19 comments · Fixed by #677
Closed

Mesh simplification after warp is often undesirable #673

wrongbad opened this issue Dec 26, 2023 · 19 comments · Fixed by #677

Comments

@wrongbad
Copy link
Contributor

It seems that co-planar triangles get automatically merged after a warp operation. But this is problematic when chaining warp operations for complex shape modeling.

Here for example, I make a detailed cylinder mesh and apply 2 warps. Notice that cone leaves most of the vertices unchanged, while horn applies a curve modifying everything.

from manifold3d import *
import numpy as np

set_circular_segments(64)

def cone(xyz):
    xyz[:,:2] *= np.clip(xyz[:,2:]/5, 0, 1)
    return xyz

def horn(xyz):
    xyz[:,:2] *= np.exp(-xyz[:,2:]/20)
    return xyz
m = CrossSection.circle(radius=10).extrude(30, n_divisions=128)
m = m.warp_batch(horn)
m = m.warp_batch(cone)

If we apply the curve op first, triangles are not coplanar, so the result is as expected.
Screen Shot 2023-12-26 at 4 56 28 PM

m = CrossSection.circle(radius=10).extrude(30, n_divisions=128)
m = m.warp_batch(cone)
m = m.warp_batch(horn)

But if we apply the linear op first, many triangles get merged and the result is unexpected.
Screen Shot 2023-12-26 at 4 56 40 PM

In this trivial example, sure user can just debug it, rearrange warps, or merge them in a single step. But in practice there can be boolean ops in between warps, and no simple workaround. And I've also run into this triggering intermittently when using warp functions like tanh that apply curves locally and asymtotically small deltas elsewhere - and those parts saw "random" merging of triangles as it probably danced around the epsilon threshold.

I'm thinking this behavior should be optional at a minimum, and possibly disabled by default. What do others think?

@elalish
Copy link
Owner

elalish commented Dec 26, 2023

Yeah, honestly I've been wondering about this myself. This is a good example of the problem, thank you. I've also had this problem when trying to apply properties to a warped mesh - the properties would keep it from being simplified, but I can't get them on in time. It would easy enough to add a public Simplify method.

Agreed, input welcome.

@zalo
Copy link
Contributor

zalo commented Dec 26, 2023

I’d find a public Simplify() method nice for various non-minkowski purposes 👍

For interactive modeling applications, it would be nice to have an explicit mechanism to keep redundant facets down for runtime speed and stability, while ensuring manifold-ness.

@elalish
Copy link
Owner

elalish commented Dec 27, 2023

For interactive modeling applications, it would be nice to have an explicit mechanism to keep redundant facets down for runtime speed and stability, while ensuring manifold-ness.

This is how our library work now, automatically. This ask to not do that in the case of warp.

@zalo
Copy link
Contributor

zalo commented Dec 27, 2023

This is diverging from the OP, but I f I were to cut an object with a new hull every frame (sweeping the convex tool from the previous frame to the current frame), would it still be able to simplify the facets on the cuts?

I was under the impression that hulls have unique Mesh IDs, which prevents the simplification process…

Either way, a method of explicitly overriding the Mesh IDs and forcing a simplification to some tolerance would be nice… unless it’s the IDing that allows it to maintain manifoldness?

And for the OP, perhaps the current auto-simplification functionality could be tied to ManifoldParams?

@elalish
Copy link
Owner

elalish commented Dec 27, 2023

@zalo - that's what #671 does. But it will only make a difference if your subsequent hulls create a bunch of coplanar triangles, which is kind of an edge case. These are the kinds of examples I'm looking for to see if #671 is actually worthwhile, so please post some comparisons there.

@zalo
Copy link
Contributor

zalo commented Dec 27, 2023

@zalo - that's what #671 does.

Err, sorry, I meant the existing auto-simplification functionality, to address the issue in the OP 🤔

@elalish
Copy link
Owner

elalish commented Dec 27, 2023

@zalo - instead of speculating, can you make an example and file a new issue with the simplification problem you see? I don't think it has anything to do with Warp (OP) and I also think it's already working as you desire.

@zalo
Copy link
Contributor

zalo commented Dec 27, 2023

Sorry, I thought you were addressing just this comment:

And for the OP, perhaps the current auto-simplification functionality could be tied to ManifoldParams?

My bad!

Here's what I was getting at with it though; replicating the OP:

TEST(Manifold, WarpSimplify) {
  Manifold m = Manifold::Extrude(CrossSection::Circle(10.0, 64), 30, 128);

  Manifold ConeHorn = m  .Warp([](glm::vec3& pos) {
                         pos.x *= std::clamp(pos.x / 5.0f, 0.0f, 1.0f);
                         pos.y *= std::clamp(pos.y / 5.0f, 0.0f, 1.0f);
                       }).Warp([](glm::vec3& pos) {
                         pos.x *= -pos.x / 20.0f;
                         pos.y *= -pos.y / 20.0f;
                       });

  Manifold HornCone = m  .Warp([](glm::vec3& pos) {
                         pos.x *= -pos.x / 20.0f;
                         pos.y *= -pos.y / 20.0f;
                       }).Warp([](glm::vec3& pos) {
                         pos.x *= std::clamp(pos.x / 5.0f, 0.0f, 1.0f);
                         pos.y *= std::clamp(pos.y / 5.0f, 0.0f, 1.0f);
                       });

  EXPECT_EQ(ConeHorn.NumVert(), HornCone.NumVert());
}

This test fails... but if I wrap the sequence in ManifoldParams().cleanupTriangles = false; (and reset it afterwards), then it passes.
So it seems like there's already a ManifoldParams field that controls the auto-simplification after warping...
(It is worth noting that ManifoldParams are not accessible from Python, however)

The hull use-case IS likely addressed with alwaysAsOriginal (which I plan to test next...)

@elalish
Copy link
Owner

elalish commented Dec 27, 2023

Good point, we did just add cleanupTriangles, which when set to false, disables the optional parts of simplify. @wrongbad that should give you a reasonable solution, though the question remains of what our default behavior should be for Warp.

@wrongbad
Copy link
Contributor Author

So I got the impression that lib manifold is marketing itself as more of a backend lib, where the users are primarily people building software on top. If that's the case I would think it's best to offer more granular and non-overlapping APIs (such as Simplify()) and not couple them with other APIs automatically. Then people building on top can choose whatever "macros" they want to combine low-level functions. Toggling global variables feels distasteful to me personally.

@wrongbad
Copy link
Contributor Author

Are you suggesting I add ManifoldParams to the python bindings?

@elalish
Copy link
Owner

elalish commented Dec 27, 2023

Yes, I agree, I don't love the system as it currently stands. Quick recap of how we got here:

  1. Simplification between Boolean ops is needed for performance, otherwise the number of extra triangles can rapidly grow. Note how much longer e.g. MengerSponge takes with cleanupTriangles=false. Also, our Booleans are automatically batched and reordered for performance, so manually calling Simplify between each would be cumbersome and inefficient.
  2. Warp didn't use to do Simplify at all, until someone warped something into a cone and noticed it had a bunch of duplicate verts, so we added it. However, I think it might be better to revert that and add a Simplify method instead.
  3. ManifoldParams is intended to be for testing only, since the global API is ugly. I begrudgingly allowed cleanupTriangles to be added because of a user who really wanted zero-volume manifolds, but it feels like a hack that I don't really want in the public API. FYI @ramcdona

@wrongbad
Copy link
Contributor Author

Interesting. I can easily imagine it would be useful sometimes to disable simplifying boolean ops too, in cases very similar to my original example here, where you create extra points intentionally so you can chain a bunch of manipulations for complex contours.

Perhaps this points to a taking an optional/defaulted argument in the modifier APIs. You could nest it in a struct, so future options don't bloat the function signature.

struct OpOptions { // TODO better name
  bool cleanupTriangles;
};

const OpOptions kDefaultBoolOptions = { true };
const OpOptions kDefaultWarpOptions = { false };

Manifold::Warp( ..., OpOptions opt = kDefaultWarpOptions);
Manifold::operator+( ..., OpOptions opt = kDefaultBoolOptions);

@elalish
Copy link
Owner

elalish commented Dec 27, 2023

You should give it a try. We never simplify out the actual intersection lines the Boolean creates, so I think it'll already do what you want.

@wrongbad
Copy link
Contributor Author

You should give it a try.

Sorry, I'm not sure what you mean.

@elalish
Copy link
Owner

elalish commented Dec 28, 2023

What I mean is, create an example of "create extra points intentionally so you can chain a bunch of manipulations for complex contours." I think I know what that means, and if I'm right, I believe it will already work the way you expect, with no need for options. If not, then I'll understand the problem better. Writing a TEST is a great way to make sure we're all on the same page.

@wrongbad
Copy link
Contributor Author

wrongbad commented Dec 28, 2023

By create extra points intentionally I'm just talking about the original example where I use n_divisions to create colinear points + coplanar triangles that later become not redundant. Already the case of just warps doesn't work, but here you can see boolean ops can have the same problem.

from manifold3d import *

def horn(xyz):
    xyz[:,:2] /= 1 + xyz[:,2:]/10
    return xyz

m = CrossSection.circle(radius=10).extrude(10, n_divisions=99)
m = m - m.translate((10,0,0))
m = m.warp_batch(horn)
Screen Shot 2023-12-28 at 2 22 45 PM

But I can abuse twist as a hack to show what I'd rather have:

from manifold3d import *

def horn(xyz):
    xyz[:,:2] /= 1 + xyz[:,2:]/10
    return xyz

m = CrossSection.circle(radius=10).extrude(10, n_divisions=99, twist_degrees=0.1)
m = m - m.translate((10,0,0))
m = m.warp_batch(horn)
Screen Shot 2023-12-28 at 2 26 48 PM

@elalish
Copy link
Owner

elalish commented Dec 28, 2023

Ah yes, I see what you mean now, thanks. I think I'd prefer to solve this problem using RefineToLength or RefineToPrecision, which I need to finally get around to implementing.

@wrongbad
Copy link
Contributor Author

Nice, thanks for the quick warp fix.

Also I was thinking RefineToLength would be a sweet feature the other day, as a means to make smoothing more useful in practice. Auto triangulation tends to generate a lot of very-thin triangles that don't smooth well.

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 a pull request may close this issue.

3 participants