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

Add Polygon simplify #138

Merged
merged 2 commits into from
Mar 18, 2021
Merged

Add Polygon simplify #138

merged 2 commits into from
Mar 18, 2021

Conversation

okcoker
Copy link
Contributor

@okcoker okcoker commented Mar 4, 2021

Added Polygon simplify.

The expected output numbers had to be altered with more precision from what you see here

I noticed this was done with the LineString tests as well.

@okcoker okcoker force-pushed the feature/polygon-simplify branch from 8bd2e1e to 93939d9 Compare March 4, 2021 10:39
@1ec5 1ec5 requested a review from Udumft March 12, 2021 17:35
@1ec5 1ec5 added this to the v2.0.0 (RC) milestone Mar 12, 2021
Copy link
Contributor

@Udumft Udumft left a comment

Choose a reason for hiding this comment

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

A few comments:

Sources/Turf/Geometries/Polygon.swift Outdated Show resolved Hide resolved
}

/**
* Returns true if ring has at least 3 coordinates and its first coordinate is the same as its last
Copy link
Contributor

Choose a reason for hiding this comment

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

This description does not match the logic of the function: if a ring has 3 coordinates, and it's first and last coordinates are equal - it will return false. Thus, the function checks first and last coordinates only for 3-points rings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes the original comment set me up for failure. Just pushed an update.

Copy link
Contributor

@Udumft Udumft left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for contributing!

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.

3 participants