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

[WIP] Ignore all flat triangles in triangulation #2248

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

andy-sweet
Copy link

This partially fixes #2247. By ignoring flat/zero-area triangles (i.e. where all three points are collinear), we avoid the assertion failure associated with overwriting the entry in _edges_lookup. But there is still an unexpected triangle and vertex in the output.

I added a few simple and similar tests to capture what I expect from the output, and where the behavior breaks down. Before the change both test_triangulate_collinear_path and test_triangulate_collinear_path_with_repeat fail with the assertion failure. After the change, only test_triangulate_collinear_path_with_repeat fails, not due to the assertion, but because a triangle is found, which also contains a vertex that is not in the input - instead I would expect no triangles to be returned.

I think this PR represents a small improvement from main for two reasons.

  1. The original assertion failure makes me think that the triangulation algorithm assumed that it would never see duplicate edges based on the way it was implemented (i.e. the state is not just exceptional, it should be impossible). This PR prevents one way of reaching that state.
  2. Before this change there was an implementation comment # ignore flat tris, which suggested that the _add_tri was trying to skip zero-area triangles. But the implementation only ignored triangles with at least 2 points that have duplicate coordinates. This PR adds a proper check for that condition.

However, I have some reservations about this PR in its current state.

  • I'm not exactly sure what to expect from the output of triangulation. It looks like we always get some vertices that weren't in the input, but in most cases they are not actually used in any of the output triangles, so that seems fine (as reflected by the tests).
  • It doesn't fix all the issues described in Unexpected triangulation results #2247 (i.e. there is still an unexpected triangle and vertex).
  • It does a little refactoring/simplification in the implementation, which I think is correct, but would be good for someone to check my geometry!

Note that some of the test cases represent degenerate cases, so I think an alternative fix here is to raise an exception with a suitable message and document that behavior. Personally, I would expect a triangulation algorithm to expect some degenerate cases and handle them fairly gracefully (perhaps with a warning), but this is my first time thinking about triangulation, so I'm very much a beginner here.

Any thoughts/comments/ideas welcome!

@andy-sweet andy-sweet marked this pull request as draft November 3, 2021 17:11
@andy-sweet andy-sweet changed the title Ignore all flat triangles in triangulation [WIP] Ignore all flat triangles in triangulation Nov 3, 2021
@SystemicArcana
Copy link

Should the assertion assert a != b and b != c and c != a be placed after the check for triangles with duplicate points? I kept on hitting a failed assert error there and moving that assertion fixed the issue for me.

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.

Unexpected triangulation results
2 participants