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

Better faces #1040

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Better faces #1040

wants to merge 13 commits into from

Conversation

elalish
Copy link
Owner

@elalish elalish commented Nov 14, 2024

I'm replacing our CreateFaces algorithm. Our previous was based on connected components, but in the case of e.g. a high-res sphere, it would connect everything into one big face. So then we added an algorithm to check global tolerance of each face after the fact, and if that failed, it would just throw out all the faces and say each triangle was individual.

This new algorithm skips all that and instead finds faces by starting with the largest triangle and searching its neighbors until none are within tolerance. Then it gets the next largest unprocessed triangle and does it again, serially, until all triangles have been assigned to faces. It's a bit arbitrary, but works decently.

Ideally we can eventually follow this on with some improved edge collapsing code.

@elalish elalish self-assigned this Nov 14, 2024
@elalish elalish requested a review from pca006132 November 14, 2024 06:07
@fire
Copy link
Contributor

fire commented Nov 14, 2024

This could be a way to attach an edge-length refiner with collapse or subdivide done per polygon.

Vec<TriPriority> triPriority(numTri);
for_each_n(autoPolicy(numTri), countAt(0), numTri,
[&triPriority, this](int tri) {
meshRelation_.triRef[tri].faceID = -1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this causing the check in line 238 and 249 to always be false?

Copy link
Owner Author

Choose a reason for hiding this comment

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

No, because they get reassigned on lines 240 and 253.

src/impl.cpp Outdated Show resolved Hide resolved
src/impl.cpp Outdated Show resolved Hide resolved
@fire
Copy link
Contributor

fire commented Dec 1, 2024

can the random selection be more deterministic for like ai.. and import recreation / reproducibility

@elalish
Copy link
Owner Author

elalish commented Dec 2, 2024

can the random selection be more deterministic for like ai.. and import recreation / reproducibility

Since this is based on sorting, it should be as deterministic as we can hope for.

@fire
Copy link
Contributor

fire commented Dec 2, 2024

Can you see what happens when you try to merge https://github.com/user-attachments/files/17969298/node_3d.zip ?

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 this pull request may close these issues.

3 participants