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

Simplifying Complex Polygons #320

Closed
ghost opened this issue Aug 4, 2014 · 12 comments · Fixed by Esri/terraformer-arcgis-parser#28 or #340
Closed

Simplifying Complex Polygons #320

ghost opened this issue Aug 4, 2014 · 12 comments · Fixed by Esri/terraformer-arcgis-parser#28 or #340
Assignees
Labels

Comments

@ghost
Copy link

ghost commented Aug 4, 2014

When using a FeatureLayer, and using the simplifyFactor option, complex polygons with holes are rendered incorrectly. I'm guessing (not sure) that this is happening because the algorithm is sampling every nth point, and perhaps not noticing if it is a point on the outer ring, or an inner ring.

See example below of the boundary of Grand Junction, CO on a desktop (simplifyFactor set to zero), vs Grand Junction on a mobile device (simplifyFactor automatically set to 0.8).

http://dola.colorado.gov/cms-base/sites/dola.colorado.gov.gis-cms/files/projects/muniproj/index.html

@patrickarlt
Copy link
Contributor

Definitely a bug, I'm posting a temporary workaround and a detailed explanation of the problem here. In order to help me with the problem could you give me the feature ids (FID) for some other features that you don't think are drawing correctly so I have more test cases?

Temporary Workaround

This seems to work if you apply less simplification (0.1 - 0.2) seems to render the features correctly while still giving a small response.

Test Cases

The test case involves making 2 requests (one simplified one not) and comparing the results.

http://jsfiddle.net/patrickarlt/B376V/

edit_fiddle_-_jsfiddle

There is definitely something up with the holes, some are filled in where they shouldn't be, most noticeably at the top of the polygon.

Compare this to the JS API output of the same thing.

arcgis_api_for_javascript_sandbox

This is probably the result of an edge case in L.esri.Util.arcgisToGeojson not handling the (really) complex polygon conversion properly.

@patrickarlt patrickarlt added the bug label Aug 6, 2014
@patrickarlt patrickarlt self-assigned this Aug 6, 2014
@ghost
Copy link
Author

ghost commented Aug 6, 2014

Here are two more examples:

FID 277: Grand Junction
var map = new L.Map("map").setView([39.064821,-108.557554], 12);

FID 174: Broomfield
var map = new L.Map("map").setView([39.940254,-105.069677], 10);
*important to start this one at zoom level 10, if started at zoom level 12 it seems to load fine.

@patrickarlt
Copy link
Contributor

@GeoDemogCO Great, now i have some good test cases I'll tackle this in a few weeks when I get back from vacation.

Until then, i would try setting simplifyFactor less aggressively. (0.1-0.2) range) you should still see some good results.

@patrickarlt patrickarlt modified the milestone: Leaflet 0.8 Aug 20, 2014
@patrickarlt
Copy link
Contributor

@GeoDemogCO Looks like that service was deleted. Could you post a link to the new service?

@ghost
Copy link
Author

ghost commented Aug 29, 2014

Patrick,

Sorry about that. Here's the new info (the geometries in question have likely not changed):

http://services.arcgis.com/IamIM3RJ5xHykalK/arcgis/rest/services/Web_Annexations08252014/FeatureServer/0

FID = 95 - Thornton
FID = 157 - Grand Junction
FID = 46 - Broomfield

@ghost
Copy link
Author

ghost commented Aug 29, 2014

Correction on the FIDs:
FID = 206 - Thornton
FID = 417 - Grand Junction
FID = 157 - Broomfield

@patrickarlt
Copy link
Contributor

@GeoDemogCO Found a fix for this and put it in #340 so it will be part of the next release.

If you need this to work ASAP you can replace the convertRingsToGeoJSON in esri-leaflet-src.js with the new one from the PR and this should work.

@ghost
Copy link
Author

ghost commented Aug 29, 2014

Thanks a ton for the fix, and for esri-leaflet in general. Very impressed!

@ricdea
Copy link

ricdea commented Dec 4, 2014

There are still problems with rendering the internal boundaries of shapes with holes in them.

E.g. Load the simplifying complex features demo page:
http://esri.github.io/esri-leaflet/examples/simplifying-complex-features.html

Scroll south a little bit

zoomed out

Zoom in on the area with the hole

The outside boundary of the smaller area is correctly rendered.

The inside boundary of the larger area is not redrawn and remains jagged.

zoomed in

@jgravois
Copy link
Contributor

jgravois commented Dec 4, 2014

thx for the feedback. i'm seeing the same thing you are so i'm going to go ahead and reopen this issue. i'll dig in and let you know what i find.

@jgravois jgravois reopened this Dec 4, 2014
@jgravois
Copy link
Contributor

jgravois commented Dec 4, 2014

the distinct simple polygon ('Freedom' - 95019) is appropriately resampled as you zoom in closer and closer, while the hole specifically in the surrounding polygon ('Watsonville' - 95076) is not.

the appopriate call is being fired to update the geometry of the existing layer

layer.setLatLngs(updateGeo.getLatLngs());`

and updateGeo._holes contains an increasing number of verticies as you zoom in. so at the moment, i'm stumped.

@patrickarlt
Copy link
Contributor

I did a little digging on this because I suspect its a Leaflet issue. Leaflet/Leaflet#2095 (comment) it looks modifying getLatLngs() to return holes was probably done as a part of the vector layers refactor for Leaflet 1.0.

To reproduce try to following code.

// make a new polygon
var p = L.polygon([
  //outer ring
  [
    [15.1171875,-7.36246686553575],
    [15.1171875,43.32517767999296],
    [68.203125,43.32517767999296],
    [68.203125,-7.36246686553575],
    [15.1171875,-7.36246686553575]
  ],
  // hole
  [
    [30.937499999999996,9.795677582829743],
    [30.937499999999996,27.059125784374068],
    [49.5703125,27.059125784374068],
    [49.5703125,9.795677582829743],
    [30.937499999999996,9.795677582829743]
  ]
])

p.getLatLngs(); // only returns one array

So only the first ring is ever updated. To solve this we could use toGeoJSON() and then setLatLngs() with the resulting geojson coordinates. I'll continue looking into this but it looks like this will be solved when Leaflet 1.0 comes out.

patrickarlt added a commit that referenced this issue Apr 28, 2015
fix #320, update using full GeoJSON coords rather then rely on getLatLngs()
jgravois pushed a commit to jgravois/esri-leaflet that referenced this issue Apr 23, 2022
jgravois pushed a commit to jgravois/esri-leaflet that referenced this issue Apr 23, 2022
fix Esri#320, update using full GeoJSON coords rather then rely on getLatLngs()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants