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

Fix issue of exporting to google map multiple polygon #127

Merged
merged 3 commits into from
May 10, 2018
Merged

Fix issue of exporting to google map multiple polygon #127

merged 3 commits into from
May 10, 2018

Conversation

songjiahong
Copy link
Contributor

  1. Fix the algorithm of checking whether a polygon is clockwise
  2. Add all vertexes including the last overlapped one to the google map polygon

@arthur-e
Copy link
Owner

arthur-e commented May 6, 2018

It looks like several tests are failing. Can you check out the Travis CI build details? In particular, the first Google Maps 3 test that fails:

FAILED TESTS:
  Standard WKT Test Cases: 
    Coverting WKT strings into objects: 
      ✖ should convert a basic POLYGON string to a Polygon instance
        PhantomJS 2.1.1 (Linux 0.0.0)
      Expected '(10, 30),(40, 40),(40, 20),(20, 10),(10, 30)' to equal '(10, 30),(40, 40),(40, 20),(20, 10)'.
      tests/wicket-gmap3-spec.js:834:35

It looks like maybe the first vertex is not being repeated (to close the ring)?

@songjiahong
Copy link
Contributor Author

Not feeling good today, I will check these tomorrow. Looks like the tests should be updated.

wicket-gmap3.js Outdated
// Orient inner rings correctly
if (polygonIsClockwise(c[j]) && this.type == 'polygon') {
verts.reverse();
if (j === 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked the logic of Google polygon, it doesn't require that the outer loop has be to counter-clockwise, the only requirement for polygon with hole is that the orientation of outer one should be different from inner ones.

@@ -336,19 +339,8 @@
return false;
}

if (l === 2) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same logic for more than 1 paths. So I remove the 2 paths check.

areas = obj.getPaths().getArray().map(function (k) {
return sign(k) / Math.abs(sign(k)); // Unit normalization (outputs 1 or -1)
return sign(k) > 0 ? 1 : -1; // Unit normalization (outputs 1 or -1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to avoid calculating of sign twice here

@@ -380,22 +371,6 @@
x: tmp.getAt(0).lng(),
y: tmp.getAt(0).lat()
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I'm not quite sure about the change, need your input here.
Does the components internally require all rings have the same orientation (clockwise or counter-clockwise)? If not, then maybe we don't need to reverse the inner rings

Copy link
Owner

Choose a reason for hiding this comment

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

Wicket doesn't care about the orientation of rings but the WKT standard requires that inner rings are reversed with respect to outer rings. For example, if the outer ring is oriented clockwise, then any inner rings should be oriented counter-clockwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think this through and update the multiple polygon check

@arthur-e
Copy link
Owner

arthur-e commented May 8, 2018

Thanks for checking the tests @songjiahong. Let me know what you've decided about inner ring orientation, then I'll confirm the pull request.

@songjiahong
Copy link
Contributor Author

All good now

@arthur-e arthur-e merged commit 2fadbc0 into arthur-e:master May 10, 2018
@arthur-e
Copy link
Owner

Great! Thanks so much.

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.

2 participants