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

Avoid adding small segment from some buffer operations #282

Closed
wants to merge 1 commit into from

Conversation

mwtoews
Copy link
Contributor

@mwtoews mwtoews commented Feb 15, 2020

This bug, described on Trac, has been around since probably the first releases of GEOS, and is also in JTS master.

Some tests needed to be modified, as there are sometimes fewer points with the revised logic.

This change will visible downstream, for instance shapely uses a default quadsegs to 16, which has this rounding error for buffered points. Other geometries and quadseg combinations may yield fewer coordinates.

Fixes #743

@dr-jts
Copy link
Contributor

dr-jts commented Feb 15, 2020

Seems like a good fix. I will apply it to JTS as well.

@mwtoews
Copy link
Contributor Author

mwtoews commented Feb 15, 2020

Seems like a good fix. I will apply it to JTS as well.

Thanks, I had a quick look at JTS to see the same logic in OffsetSegmentGenerator.java, but I'm out of my league when it comes to Java development.

@mwtoews
Copy link
Contributor Author

mwtoews commented Feb 16, 2020

And yup, this bug had been around for 17 years (seen here), but has only been bothering me for 5 years.

@dr-jts
Copy link
Contributor

dr-jts commented Feb 16, 2020

JTS has logic to remove very small segments, but only for curves generated for non-point geometries. The point buffer generation uses the same code to generate "fillets", but for a full circle. This is why there is no check for short segments in the fillet code itself.

An alternative to your patch is to switch to using a better generator for circles, which would just generate the circle with exactly the desired number of points. I might consider implementing that instead (just because it's more direct logic, and stays out of the way of the code dropping short segments).

@mwtoews
Copy link
Contributor Author

mwtoews commented Feb 16, 2020

@dr-jts feel free to supersede this PR with something better!

@Algunenano
Copy link
Contributor

I remember fixing something like this in Postgis with a simpler solution: instead of calculating the increment and adding it in each iteration (which increases errors) you can keep the first and last point in the angle (already known) and calculate each intermediate point as n/segments * angle. It's going to require some extra ops per iteration but the error doesn't accumulate.

@dr-jts
Copy link
Contributor

dr-jts commented Mar 23, 2020

See JTS PR 526. It uses the nice approach suggested by @Algunenano (which is the standard pattern for avoiding accumulating numerical error during incremental iterations).

@pramsey
Copy link
Member

pramsey commented Mar 24, 2020

Closing this in favour of #296

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.

None yet

4 participants