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

Remove degenerate polygons in PolySetBuilder #5007

Merged
merged 4 commits into from Mar 17, 2024
Merged

Conversation

kintel
Copy link
Member

@kintel kintel commented Feb 22, 2024

Also includes some PolySetBuilder refactoring.

Fixes #5000

Note: rotate_extrude() could be improved to better handle manifold corner cases. Deferring to #5046

@kintel
Copy link
Member Author

kintel commented Feb 22, 2024

@pca006132 In case you have any comments :)

p->vertices=this->points;
p->indices=this->faces;
for (auto &poly : p->indices)
PolySetBuilder builder(this->points.size(), this->faces.size());
Copy link
Member

Choose a reason for hiding this comment

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

going through PolySetBuilder will lose manifoldness.

Copy link
Member Author

Choose a reason for hiding this comment

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

Riight, this is the wrong hammer for this job; we need something that cleans up a user-specified polyhedron. I'll think about it, perhaps just tessellate to triangles early for user polyhedrons.

void addVertex(int n);
void addVertex(const Vector3d &v);
// Calling this is optional; will be called automatically when adding a new polygon or building the PolySet
void endPolygon();
Copy link
Member

Choose a reason for hiding this comment

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

why should we have this if it can be called automatically?

Copy link
Member Author

Choose a reason for hiding this comment

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

Really just for API cleanliness, as most client code will use begin/end pairs to make it more readable.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could make it required and rather assert if someone tries to do two beginPolygon() in a row.

Copy link
Member

Choose a reason for hiding this comment

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

We could make it required and rather assert if someone tries to do two beginPolygon() in a row.

Sounds good.

@pca006132
Copy link
Member

I think we probably should not do this in the PolySetBuilder, e.g. manifold outputs can have degenerate triangles but we don't want to pass it through PolySetBuilder as this loses manifoldness as well.

@pca006132
Copy link
Member

Another concern (not specific to this PR): we are using assert for checks, but we do not build in debug mode in the CI, so these checks are not run automatically.

@kintel
Copy link
Member Author

kintel commented Feb 23, 2024

I think the only issue here is PolyhedronNode. Apart from that, this only affects code already using PolySetBuilder, and it's a cheap feature. The goal of the assert() is to catch programming errors, not run-time errors, so it's really just a convenience for developers.

That said, there might be a better/more general place to insert a polygon cleaner which doesn't collapse manifolds.
Btw., do you know a good nomenclature to describe this type of objects (manifolds where multiple different vertices have the same position in space)?

@pca006132
Copy link
Member

Btw., do you know a good nomenclature to describe this type of objects (manifolds where multiple different vertices have the same position in space)?

No, but @elalish may know

@jordanbrown0
Copy link
Contributor

we are using assert for checks, but we do not build in debug mode in the CI, so these checks are not run automatically.

Tidbit: It's been a long time, but the last time I tried to build in debug mode for MSYS2/Windows, the build failed - it hit some kind of Windows symbol table limit.

@elalish
Copy link

elalish commented Feb 24, 2024

I'm not sure it has a specific name - it's just an artifact of various different manifoldness definitions floating around. I wrote up the major differences on our wiki. The short answer is that anything that doesn't allow disjoint vertices at the same location cannot maintain manifoldness through Boolean operations in general. It's closely related to why STL is not capable of reliably transmitting manifolds either. Hence why 3MF should be used instead, and now even glTF with my new EXT_mesh_manifold extension.

@kintel kintel merged commit 5c78196 into master Mar 17, 2024
5 checks passed
@kintel kintel deleted the kintel-stl-export branch March 17, 2024 00:38
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.

Crash in STL export after rotate_extrude
4 participants