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

Calculate winding from ring. #343

Merged
merged 1 commit into from
Sep 10, 2019
Merged

Calculate winding from ring. #343

merged 1 commit into from
Sep 10, 2019

Conversation

akater320
Copy link
Contributor

Correctly calculate the winding of triangles.

Fixes 342

Signed-off-by: Adam Kater [email protected]

@nprigour
Copy link
Contributor

Indeed the suggested commit fixes the issue mention in #342 and permits creation of holes with 3 coordinates and suppress the thrown exception.
However @akater320 would it be possible to explain a little bit the issue with the winding? Debugging I can infer that all holes are converted to CW (ClockWise) winding. Do you think that there is a particular reason for doing this or is it just an arbitrary assumption by the class creator in order all holes to have the same winding? Would there be any implication that we must be aware of?

@akater320
Copy link
Contributor Author

Good question.
I think the author's intent was to orient the edges so that the interior of the polygon is always on the right.

But, assuming this is the correct Polygon class com.vividsolutions.jts.geom.Polygon the comments explicitly state that orientation of rings does not matter. There is also no guarantee that orientation is preserved.
Java 2D uses an even/odd ray-cast to determine the interior of closed Paths so orientation won't matter there either.

So, the winding direction should probably be treated as arbitrary.

@nprigour
Copy link
Contributor

OK thanks for the clarification @akater320.
Now in order to finalize the issue I think (please @fgdrf confirm) that we should add a test that verifies that correct behavior of GeometryCreationUtil when creating polygon with a hole with 3 coordinates. There is a test class GeometryCreationUtilTest in plugin org.locationtech.udig.tool.edit.tests which can be extended to include a test on that. Would it be possible to do it?

@akater320
Copy link
Contributor Author

@nprigour Is it possible to run all of the tests at once? Or is it necessary to run each test plugin individually?

@nprigour
Copy link
Contributor

What do you mean by all the tests at once? If you refer to all udig tests in the various plugins then this can be accomplished by mvn by running the following command
mvn clean install -Pproduct,test

@akater320
Copy link
Contributor Author

@nprigour
Perfect! Thank you.

Copy link
Contributor

@fgdrf fgdrf left a comment

Choose a reason for hiding this comment

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

@akater320 Were you able to run tests? Are you working on a test-case?

@@ -248,7 +248,7 @@ public static Polygon createPolygon( EditGeom currentGeom ) {
}
LinearRing hole = GeometryBuilder.create().safeCreateLinearRing(coordArray);
// FIXME test when the hole has only one coordinate.
Copy link
Contributor

Choose a reason for hiding this comment

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

Any chance to address the missing check for holes with only one coordinate?

@fgdrf
Copy link
Contributor

fgdrf commented Sep 10, 2019

Just testing a maven run with test profile, if I get no failures I'll merge it

@fgdrf fgdrf added this to the 2.1.0 milestone Sep 10, 2019
@fgdrf fgdrf merged commit 99bf03c into locationtech:master Sep 10, 2019
@fgdrf fgdrf added the bug label Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hole Cutter tool cannot implicitly close triangular holes
3 participants