Remove CreateFaces entirely #951
Conversation
src/manifold/src/constructors.cpp
Outdated
| impl->CreateTangents(impl->UpdateSharpenedEdges(sharpenedEdges)); | ||
| const Vec<bool> internalEdges = impl->InternalEdges(); | ||
| impl->CreateTangents(impl->UpdateSharpenedEdges(sharpenedEdges), | ||
| internalEdges); |
There was a problem hiding this comment.
@pca006132 Any ideas for reducing the duplication between these two MeshGL functions?
|
Btw, do you have any particular test where
|
|
Huh, I thought it was slow, but it was awhile back - maybe your union find optimization helped more than I realized. I believe in general the slowness comes with importing a large mesh. Anyway, the primary purpose of this PR isn't speed, but making the mesh relation work better. |
|
Btw, I guess this is the final breaking change (feature-wise) for 3.0? Others are mostly some API cleanup, as well as binding changes to match our updated APIs? |
Close, but I think removing GLM from our public API is one more: #962. |
|
Hmm, definitely still some work to do on this one. Something's still a bit broken because it's not collapsing all edges it should. However, it's also dramatically slower, which has me a little more worried - may need to change the approach. @pca006132 if you want to have a look, maybe see if Tracy has any interesting insights (since I can't run it on Mac), that would be great. |
|
Do you have any tests that are slower in particular? I tried running some tests, but they fail with vec out of range immediately, so it is hard to get profiler result for this. And I think tracy does support mac? |
|
Oh, did you pull my latest? I thought I fixed the vec out of range stuff. I guess I should try Tracy again, maybe I'm remembering wrong. |
|
Yeah, apparently I forgot to pull the latest. Will test later tonight. |
|
Just had a look at |
|
Ha, looks like #975 fixed the slowness - sure enough it just wasn't running parallel. That makes me feel better - now I just need to debug. |
This is a sizable refactor and moderate breaking change. Manifold will no longer use equal
faceIDs to mean that those triangles are coplanar. This was always problematic, because it meant that using Warp or Refine would destroy the entire mesh relation, making material association quite difficult. This also means we can remove any concept of property tolerances from our API, which I never liked. Now edges can collapse if the two triangles are coplanar, have the same meshID, and reference the same propVerts on both sides of the edge.Hopefully this will also make things a bit faster, since
CreateFaceswas always slow due to the connected components algorithm. I'm still on the fence as to whether we should store and update theinternalEdgeslist, or generate it on the fly. For the first pass it's on the fly. This PR is very much WIP.