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

Simplify (generalize) admin borders #3103

Merged

Conversation

matthijsmelissen
Copy link
Collaborator

No description provided.

@matthijsmelissen
Copy link
Collaborator Author

Be careful with testing: for some reason Kosmtik has problems handling a change in simplify algorithm. It seems it does not detect the change, and caches the old value. Before and after testing this PR, it is therefore necessary to quit Kosmtik, rather than just checkout the other branch with Kosmtik running.

@matthijsmelissen
Copy link
Collaborator Author

Before/after:
screen shot 2018-03-05 at 02 03 31 screen shot 2018-03-05 at 02 09 20

Before/after:
screen shot 2018-03-05 at 02 06 09 screen shot 2018-03-05 at 02 06 35

@kocio-pl
Copy link
Collaborator

kocio-pl commented Mar 5, 2018

The effect is subtle, but I like it. It's also safe to use it, because this function is available since API 2.2.0.

Poland (click on the images to view the full size):

z6
Before
8zakw0dt
After
echyd916

z7
Before
v8jqipma
After
gvviqwh6

@imagico
Copy link
Collaborator

imagico commented Mar 5, 2018

For reference for those who are not aware of it - this is essentially a re-submission of an idea that has been discussed and ultimately dismissed in #907 and #1938 - technical implementation differs but functionally it is the same.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Mar 5, 2018

Thanks for the links - a lot of reading, though, because I don't even remember what I was thinking back then. This was a prohibitively big problem with performance, right? However if this solution would fly, I would also like to test it on natural reserve borders.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Mar 8, 2018

For reference for those who are not aware of it - this is essentially a re-submission of an idea that has been discussed and ultimately dismissed in #907 and #1938 - technical implementation differs but functionally it is the same.

The question is why were they dismissed - because of functionality or technical implementation?

This #1938 (comment) is about severe performance problems ("I had to revert this pull request, since it was bringing the OSMF servers to a halt") - the code however has been merged before, so I wouldn't even call it "dismissed". I have yet to look at #907.

@matthijsmelissen What do you think about it?

@matthijsmelissen
Copy link
Collaborator Author

Exactly: #1938 was reverted for a technicality, namely the performance issues of ST_Within. That doesn't play a role here, and it seems to be running fine on @rrzefox' server.

@yohanboniface
Copy link

yohanboniface commented Mar 8, 2018

FYI, while testing line-simplify on another project, I had some glitches on big polygons, like this (here on zoom 4):

screenshot

Never investigated until the final word, but just a warning so you double check on your side :)

Edit: this appears when mixed with line-clip

@matthijsmelissen
Copy link
Collaborator Author

In the demo renderings, I have seen no evidence we'll run into this. Thanks for mentioning it though!

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.

4 participants