Support Complex Polygons

[DavidHudlow]

Fixes issue #1121 by supporting the complex polygon in the given file.

[pjcozzi]

@DavidHudlow I'd still like to review this in more detail, but I made some updates so that it works with recent changes we made to our Cartesian types (#1120).

To get my changes, first merge the main Cesium repo into your branch. There will be a couple of trivial conflicts. Fix them and commit. Then merge my complexpolygon. Here is the commit with my changes: dc703f9

[pjcozzi]

You'll also want 2183e58, which fixes a terriable mistake I made. How hard is it to add a test case for this?

[pjcozzi]

Yes, indeed. Looks like the original code intended to compare z. @DavidHudlow is that right?

if (s2.cross(s1) < s3.cross(s4).z) {

[pjcozzi]

@DavidHudlow would it be better to always use the same seed for this algorithm so that we get deterministic results? For example, for files like https://raw.github.com/turban/N2000-Kartdata/master/NO_Admin_latlng.topojson. What is the benefit of using a different seed?

[DavidHudlow]

@pjcozzi Yes, that should have had the .z.

rseed (perhaps not really a seed?), must change before each new random number is generated so that the new number will be different from the last. It can be manually reset using resetSeed() before beginning triangulation of a new polygon. There is no disadvantage to this. The reason I changed the algorithm to use this random algorithm rather than Math.random() was so that the "seed" could be reset for testing for deterministic results.

[DavidHudlow]

@pjcozzi Are you still seeing failures with that file? I haven't been able to reproduce them, so maybe your fix to that cross product comparison fixed the problem?

[DavidHudlow]

Also, regarding adding test cases for changes to the algorithm, unfortunately, it's difficult to come up with cases that will predictably cause the algorithm to fail because of the nature of randomly trying to find valid divisions. If a flaw in the algorithm causes some cuts to be branded invalid when they're valid, there may be alternatives found that cause the triangulation to still succeed.

Likewise if a division is considered valid based on one criteria (say, it's deemed internal when it's really not), it will often be correctly branded invalid because it crosses a side of the polygon.

The other difficulty is that although you can triangulate a polygon 100 times to see if it ever chokes, you can't automatically determine if each result was actually valid (because the order of the triangles is different each time).

Having said that, it would certainly be possible to create some more "special case" tests that have a higher tendency to reveal problems. It might also be possible to create some good cases and iterate (or generate) every possible valid result for more thorough testing.

[mramato]

David, I'm not very familiar with the polygon code, but it's possible for us to generate random, but repeatable numbers in the tests. Before each test use CesiumMath.setRnadomNumberSeed(0) and then have your code call CesiumMath.nextRandomNumber instead of the RNG you are using now. This ensures that your get the randomness you need, but it will create repeatable tests. See the bottom of https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Core/Math.js

[pjcozzi]

@DavidHudlow can you fix the doc build in your branch?

[java] js: "jsdoc.js", line 86: exception from uncaught JavaScript throw: TagValueNotPermittedError: The @private tag does not permit a value. File: PolygonPipeline.js, Line: 420

I think we need to remove the ] on Line 430.

I am with @mramato about generating repeatable random values. My question was can we do this all the time - not just for tests - so we have consistent runtime behavior. It sounds like we can, right?

With NO_Admin_latlng.topojson, I still got the error on my 3rd try running out of your fork.

[DavidHudlow]

@mramato Yes, I'll definitely change it to use CesiumMath's random number generator, and have it reset the seed before beginning each polygon triangulation. I'm not sure if that was available when I first changed the triangulation algorithm, but if it was, I wasn't aware of it.

@pjcozzi I'll try to track that down.

[emackey]

The random number thing is indeed new. My vote would be to leave the seed alone, so it can be user-controlled when desired. This would result in different default behavior when files are loaded in a different order, but it would still be repeatable behavior, and the user could pick seeds that work better for special cases.

[mramato]

David (and Ed) rather than use the CesiumMath functions, me and Cozzi feel
you should require in mersenne-twister (from Source/ThirdParty) into your
Polygon code. Then at the beginning of triangulation, you create a new
instance, always using the same hardcoded seed (0 is fine). This means
that the same polygon will always use the same random values, no matter the
order or which application it is used it; that is ultimately the desired
behavior.

On Thu, Sep 26, 2013 at 4:10 PM, Ed Mackey notifications@github.com wrote:

The random number thing is indeed new. My vote would be to leave the seed
alone, so it can be user-controlled when desired. This would result in
different default behavior when files are loaded in a different order, but
it would still be repeatable behavior, and the user could pick seeds that
work better for special cases.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1163#issuecomment-25199587
.

[pjcozzi]

@mramato is spot on.

[DavidHudlow]

That last commit is a bit thrown together. Not sure if it'll fix all cases, but it seems to fix the issue with the file we were testing. I'm a little concerned about the performance of these additions though. I'm afraid I may have hurt us there. I'll try to investigate that, but it may be hard to get it done before the next release.

[pjcozzi]

Can you please merge in the main repo's master? In general, when ever the GitHub pull request has the following we need to merge in the target branch:

[pjcozzi]

This fix seems to work well in Chrome, but it failed in Firefox on the first try.

Firefox is also, of course, way slower to triangulate.

[pjcozzi]

Back of the napkin numbers shows performance to be about the same.

[pjcozzi]

@DavidHudlow I'll close this for now, but please reopen when you are ready. We can track with #1209, which is now an enhancement, not a bug fix.