-
Notifications
You must be signed in to change notification settings - Fork 98
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pca006132 Any ideas for reducing the duplication between these two MeshGL
functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this to impl?
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. |
I noticed you mentioned tracy, samply has a nice integration with the mozilla (web) tools for profiling. |
This is looking like a larger can of worms than I wanted. I'm closing for now in favor of a reduced-scope version in #991. I might come back to this someday since it seems like it should have some promise for speed improvements. |
This is a sizable refactor and moderate breaking change. Manifold will no longer use equal
faceID
s 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
CreateFaces
was always slow due to the connected components algorithm. I'm still on the fence as to whether we should store and update theinternalEdges
list, or generate it on the fly. For the first pass it's on the fly. This PR is very much WIP.