Don't simplify unless requested #1185

elalish · 2025-03-09T09:58:02Z

Fixes #1138 - really that thread, while interesting, has grown way too long and gone on too many tangents. Some has already been fixed, this fixes some more - please make separate issues for whatever's left.

Now that #1147 has landed which makes the Boolean only decimate the intersected parts of meshes, this PR finishes the job by removing our auto-decimation from the rest of our functions, so users can just call it manually if they want through Simplify. I've also deprecated the cleanupTriangles option, since it's effectively always false now. We're still collapsing degenerate triangles and fixing topology at the epsilon level, but not touching colinear edges, coplanar faces, or anything that's only within tolerance. @stephomi, @starseeker, @howardt I believe you all may have asked for things along this line at various points, so it would be great to have some early eyes on this and get feedback.

Good new / bad news is this improved our testing and thus found that Simplify doesn't handle properties correctly at all. It looks like I now need to add a significant refactor here to fix that, but on the plus side I think I can remove our CreateProperties function entirely, which has gone through quite a bit of optimization due to slowing things down a lot in the past. I think I have a cleaner way now.

…eDegenerates

pca006132 · 2025-03-14T12:36:22Z

btw this may be related to simplification: openscad/openscad#5738

…eDegenerates

elalish · 2025-03-16T10:12:08Z

src/boolean_result.cpp

+  const int numProp = std::max(inP_.NumProp(), inQ_.NumProp());
+  if (numProp > 0) {
+    // Ideally, switch over from triProperties vec to Halfedge.startProp - then
+    // the prop index will be available in the append edges functions below.


@pca006132 I'm struggling to get this one finished - are you interested in taking a look? If you look at my last commit, I've added comments on the refactor I have planned. I think it's pretty straight forward, and I bet you'd optimize it better than I would.

I ask because this is probably the last major PR to land before doing a release, right? We have a lot of good stuff now (mostly yours) and I don't want to delay it. Let me know what you think - and is my description clear at all?

Sure, I can have a look at it later tonight.

So this is kind of similar to what CreateProperties is doing, but you want to do it in parallel, and probably handle edges differently?

About the inclusion number array, should it be indexed by the vertex id, halfedge id, prop vert id or something?

Yes, index by prop vert. Every prop vert has exactly one vert, so that makes things nice. The idea is to keep track of properties directly by index just like the verts, instead of needing to look anything up with maps or hash tables.

pca006132 · 2025-03-17T12:47:11Z

I still have several high-level questions about how properties work, but I am getting there.

First, to make sure I understand this correctly, a prop vert is numProp many double entries in the MeshRelation::properties vector.

What we want to do here is to

For retained vertices from either P or Q, copy and duplicate them according to the inclusion number i03 or i30.
Question: Should we duplicate the properties entry, or reuse the same index?
If we reuse the same index, does that mean what matters to us is whether the inclusion number is zero?
If not, when will we have cases where the endpoints of an edge share properties?
For new vertices introduced due to face-edge collision, somehow create new properties. Questions:
1. If each vertex can map to multiple prop verts, how can we know how many prop verts there are per vertex? I don't see this information stored anywhere, and the prop2VertP/prop2VertQ initialization seems to imply that each vertex can only map to one prop vert.
2. And if each vertex can have multiple prop verts, when we copy retained vertices or perform interpolation on new vertices, should we also copy all of their prop verts? Will this make the number of prop verts explode if we apply many boolean operations?
3. I don't understand how we should apply interpolation if we add 2 properties for edges where the endpoints don't share both properties.

What I understand from the CreateProperties function is that, for newly created vertices, we find the face that created it, take the properties on the three vertices there, and interpolate using the Barycentric function. But this doesn't care about which edge causes the collision.

elalish

I made separate threads to answer each of your questions.

elalish · 2025-03-18T05:29:52Z

src/boolean_result.cpp

+    //
+    // New verts create either 2 or 3 new properties: one for the face, plus one
+    // for an edge where start and end verts share properties, or plus two for
+    // an edge that doesn't share both properties.


This is the answer to question 2-i. It might be clearer if I said "prop verts" instead of "properties" - that's what I mean.

elalish · 2025-03-18T05:35:05Z

src/boolean_result.cpp

+                 for (const int i : {0, 1, 2}) {
+                   prop2VertP[inP_.meshRelation_.triProperties[tri][i]].store(
+                       inP_.halfedge_[3 * tri + i].startVert);
+                 }


Another piece of 2-i: No - every propVert maps to exactly one vertex, hence this vector being possible. Whenever the same value shows up multiple times in this list, that's a vertex that's associated with multiple propVerts.

This is an important feature (because it's also implicitly a restriction in MeshGL), so to answer question 1 - we definitely need to duplicate rather than reuse the index. Again, this duplication is actually exceedingly rare in practice since it's only for handing self-intersecting meshes.

elalish · 2025-03-18T05:43:13Z

src/boolean_result.cpp

+    //
+    // Copy included inP and inQ properties to outR, and duplicate for new verts
+    // accoding to the inclusion numbers. Calculate new outR properties using
+    // the Barycentric function from CreateProperties.


For 2-iii: It's best to think of propVerts as being on a halfedge or triangle - the paired halfedge usually, but doesn't always, shares the same propVerts. Only if both ends of the paired halfedges share propVerts can you interpolate that as a single new shared propVert - otherwise you'll be creating two new propVerts, one for each halfedge. It's not possible to have two ends of the same halfedge have properties that don't represent the same channels of data. The channels of data are consistent across each meshID, or equivalently, each triangle run in MeshGL. So they are also consistent over a triangle or halfedge.

Can you define what share propVerts means? If each propVert maps to exactly one vertex.

And for these new vertices, where do their propVerts come from? Say, if each vertex of the associated triangle originally maps to 2 propVert, we should have 2 propVert interpolated using barycentric coord, plus some propVerts interpolated using propVerts associated to endpoints of the edge?

By share propVerts, I mean the paired halfedges point to the same prop index. Basically verts must be manifold, while propVerts may be manifold. The edge is prop-manifold if the propVerts are shared.

The second part isn't quite right. Any given triangle only has one set of three propVerts; new propVerts on either its edges or its center all get interpolated barycentricly using that single interpolant. So e.g. 6 triangles ajoin a vertex - this vertex might have 2 propVerts, meaning e.g. 3 of those triangles point to one propVert and 3 point to the other.

If two paired halfedges (with different starting vertices) can point to the same prop vert, what does it mean by each prop vert maps to one vert?

Or does this mean each prop vert maps to one edge (can be one or two halfedges)?

elalish · 2025-03-18T05:44:05Z

src/boolean_result.cpp

+    // an edge that doesn't share both properties.
+    //
+    // Copy included inP and inQ properties to outR, and duplicate for new verts
+    // accoding to the inclusion numbers. Calculate new outR properties using


For 2-ii: Yes, we copy all retained propVerts, just like we do verts. But the number isn't going to explode any more than the number of verts does. Well, technically you'll get double the number of new propVerts as new verts, but considering in general I think of these problems as scaling such that the number of new verts is O(sqrt(n)) for n: the number of input verts, I don't think that's a huge problem. Plus it's fundamentally necessary in order to represent e.g. UV coordinates.

pca006132 · 2025-03-18T15:17:16Z

src/boolean_result.cpp

+               });
+    // Scan the properties inclusion numbers, e.g. + i03[prop2VertP[prop]].
+    //
+    // New verts create either 2 or 3 new properties: one for the face, plus one


And also, how are these new prop verts computed and attached to halfedges?

For the face prop vert, it is barycentric interpolation from the three prop vert of the intersecting face, but which new halfedge to attach to?

For prop vert corresponding to an edge (two different cases), how should we interpolate them and where should we attach the prop vert to?

Sorry for a lot of questions, it may be easier for you to directly implement it rather than explaining them.

don't simplify unless requested

Loading
Loading status checks…

8f58c90

elalish self-assigned this Mar 9, 2025

elalish mentioned this pull request Mar 13, 2025

CSGMesh3D is not working in 4.4 stable godotengine/godot#104048

Open

Merge branch 'master' of github.com:elalish/manifold into onlyCollaps…

151d7e6

…eDegenerates

elalish added 2 commits March 16, 2025 21:49

Merge branch 'master' of github.com:elalish/manifold into onlyCollaps…

d466a91

…eDegenerates

added algorithm comments

Loading
Loading status checks…

36692bc

elalish commented Mar 16, 2025

View reviewed changes

elalish commented Mar 18, 2025

View reviewed changes

pca006132 reviewed Mar 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't simplify unless requested #1185

Don't simplify unless requested #1185

elalish commented Mar 9, 2025

pca006132 commented Mar 14, 2025

elalish Mar 16, 2025

pca006132 Mar 16, 2025

pca006132 Mar 16, 2025

elalish Mar 16, 2025

pca006132 commented Mar 17, 2025

elalish left a comment

elalish Mar 18, 2025

elalish Mar 18, 2025

elalish Mar 18, 2025

elalish Mar 18, 2025

pca006132 Mar 18, 2025

elalish Mar 18, 2025

pca006132 Mar 18, 2025

elalish Mar 18, 2025

pca006132 Mar 18, 2025

Don't simplify unless requested #1185

Are you sure you want to change the base?

Don't simplify unless requested #1185

Conversation

elalish commented Mar 9, 2025

pca006132 commented Mar 14, 2025

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

pca006132 commented Mar 17, 2025

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