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

Separate coplanarity from faceID #991

Merged
merged 6 commits into from
Oct 19, 2024
Merged

Separate coplanarity from faceID #991

merged 6 commits into from
Oct 19, 2024

Conversation

elalish
Copy link
Owner

@elalish elalish commented Oct 16, 2024

Simpler alternative to #951, since it doesn't change the logic we use for edge collapsing.

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.

@elalish elalish self-assigned this Oct 16, 2024
@elalish elalish added the breaking change API changes for major versions label Oct 16, 2024
@elalish elalish added this to the v3.0 milestone Oct 16, 2024
@elalish elalish changed the title [WIP] Separate coplanarity from faceID Separate coplanarity from faceID Oct 18, 2024
@elalish
Copy link
Owner Author

elalish commented Oct 18, 2024

@pca006132 You're right, I can't find any noticeable performance impact from CreateFaces anymore, so I've gone ahead and put it back into the functions I removed it from a few PRs back. Nice job optimizing it!

/// Optional: Length NumTri, contains the source face ID this
/// triangle comes from. When auto-generated, this ID will be a triangle index
/// into the original mesh. This index/ID is purely for external use (e.g.
/// recreating polygonal faces) and will not affect Manifold's algorithms.
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question: should we rename our public faceID, since it really has nothing to do with faces now? Maybe triID or triIndex?

The worst part is now in our internal TriRef struct we have faceID which really is a face ID and tri which is the internal analogue to this public faceID. That's pretty much bound to confuse someone eventually, probably me.

Copy link
Collaborator

@pca006132 pca006132 Oct 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, triID sounds better fit here. We should add some documentation for the internal faceID to clarify things a bit as well. It seems that it is already documented.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since that will be a big change across all of our binding surfaces and such, I'll leave that to its own PR.

triRef[tri] = {meshID, meshID, tri};
[meshID, keepFaceID, &triRef](const int tri) {
triRef[tri] = {meshID, meshID, tri,
keepFaceID ? triRef[tri].faceID : tri};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will happen if we always keep face ID?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be weird - for a regular initialization, that would mean every faceID was zero, which would imply the whole object was a single face. It would probably confuse something at some point.

@elalish elalish merged commit 54517b7 into master Oct 19, 2024
19 checks passed
@elalish elalish deleted the triID2 branch October 19, 2024 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change API changes for major versions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants