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

Allow degenerate triangles in polygon triangulation when necessary. #17129

Merged
merged 1 commit into from
Mar 1, 2018

Conversation

nical
Copy link
Contributor

@nical nical commented Mar 1, 2018

Fixes #17102.

This modifies the ear-clipping implementation so that it first tries to avoid completely flat triangles, but falls back to allowing them as a last resort when triangulation would otherwise fail.
I explained this with a few more details on the issue.

This adds a bit of unfortunate branchiness that hurts code aesthetics more than actual performance, because these branches are very predictable and ear clipping is already pretty inefficient anyway.

If we don't care about degenerate triangles at all, we can replace this change by simply turning the first check in Triangulate::snip into:

	if (-CMP_EPSILON > (((Bx - Ax) * (Cy - Ay)) - ((By - Ay) * (Cx - Ax)))) return false;

Using a negative epsilon instead of zero conservatively avoids potential floating point precision issues.

@poke1024
Copy link
Contributor

poke1024 commented Mar 1, 2018

Nice to see this. I looked at #16423 recently, and found out that the monotone triangulator always crashes; I then used Triangulate_EC instead (see file attached to this comment), which works, but is considerably slower than the current ear clipping algorithm fixed in this PR (6.3 ms per call vs. 2.1 ms per call with the current clipping). So in short, it's nice to see the fast current ear clipping algorithm get fixed.

@nical If I understand correctly this fixes duplicate points as well as collinear points as in ((0, 0), (10, 0), (20, 0), (50, 70))?

triangulate.cpp.zip

@nical
Copy link
Contributor Author

nical commented Mar 1, 2018

Too bad that the 3rd party monotone implementation isn't very good. It's a tough algorithm to get right but I think it's the best compromise between speed and triangulation quality when implemented correctly.

@nical If I understand correctly this fixes duplicate points as well as collinear points as in ((0, 0), (10, 0), (20, 0), (50, 70))?

Yes, I did a quick test with this shape and it works. I also tried with duplicate points, for example: ((0, 0), (10, 0), (10, 0), (10, 0), (10, 0), (20, 0), (50, 70)), and it manages to finish triangulating (you get some degenerate triangles with several vertices at the same position but it does manage to triangulate the shape).

@akien-mga akien-mga added this to the 3.1 milestone Mar 1, 2018
@hpvb
Copy link
Member

hpvb commented Mar 1, 2018

Tested against the testcase in #17102 as well as #16395 I couldn't find any regressions with playing about with other polygon shapes.

@hpvb hpvb added the cherrypick label Mar 1, 2018
@hpvb hpvb merged commit 334b6c6 into godotengine:master Mar 1, 2018
@hpvb hpvb removed the cherrypick label Mar 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants