-
Notifications
You must be signed in to change notification settings - Fork 106
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
polygons are inconsistent with geojson standard #45
Comments
Sure. A orb.Ring is just a slice/array. You can create it with 0, 1, 2, 3, 4... points. There is no validation. Not really sure what to do about it. Error on marshal? some sort of validator? |
The issue I had was on clipping. Maybe clip library should validate?
|
Hi! Loving the package in general but I got bit by this issue where the GeoJSON support is just dummy marshaller without validation. In my case, the GeoJSON geometry didn't follow the right hand rule, so I had to manually sort it CounterClockWise (the ring represented a solid polygon). I think it would make sense to have better GeoJSON capability. While the as-is marshal/unmarshal can come in handy in some cases, maybe it would make sense to have a validator and helper functions to generate standard-compliant GeoJSONs? Or maybe this issue can be closed by pointing to another package that has such helpers. For reference, what I did (and why I think it would be nice to have helpers to do these things): // r is an orb.Ring with a solid polygon, no holes
// Sort CounterClockwise
sortCCW(r)
// Close the ring by adding the first coordinate again at the end, after sorting
r = append(r, r[0])
geometry := geojson.NewGeometry(r) // Now it works Where // I didn't find this in the orb lib, but basically sort the ring points in
// counter-clockwise order starting on the southwest, to fulfill the GeoJSON
// spec.
// This is a simplistic algorithm that I came up with for a PoC, no considerations
// to performance and it might not cover all cases.
// This does it in place, side-effects galore and blabla.
func sortCCW(r orb.Ring) {
// Bounding box center, just approximate
center := r.Bound().Center()
sort.Slice(r, func(i, j int) bool {
p1, p2 := r[i], r[j]
if south(p1, center) == south(p2, center) {
// Both north, or both south
if south(p1, center) {
// Western wins
return p1.Lon() < p2.Lon()
}
// Eastern wins
return p1.Lon() > p2.Lon()
}
// Not both north or south, south wins
return south(p1, center)
})
}
func south(p, c orb.Point) bool {
return p.Lat() < c.Lat()
} |
Heya, I found this issue when suffering similar issues yesterday. It seems like there need to be some changes made to generate standard-compliant https://datatracker.ietf.org/doc/html/rfc7946#section-3.1.6
for each ring in the polygon where
for each ring in the polygon where Regarding where in the codebase to implement this, I feel like there's two options:
... and both of these options are not great 😆 Probably 2. is better, we could avoid the copy when not required, else create a copy and mutate it and then use that copy for marshalling? I'd be happy to assist in this if it's a PR you'd consider merging? |
There's also this Antimeridian Cutting rule and enforcing WGS84 but they are a task for another day 😆 |
Hi! I got to the same conclusion, so I didn't do a PR and are handling this externally with wrappers around the lib (similar to the above example code, #45 (comment)). However, not totally sure this is wanted, as per #45 (comment), perhaps the lib scope is just to be low level and have a If we want this in the lib, I don't mind doing a PR for option 2, where we just modify on a copy at marshalling time, but otherwise leave the polygon as-is, or, go for option 1, where the user needs to call a |
FWIW, I see As such I would put all the 'geojson stuff' in This approach would work well for implementing other mutating features such as Antimeridian Cutting where IMO the original |
I can issue a PR to the geojson package within 1-2 weekends, as $DAYJOB has priority and I have some tight deadlines now 😄 |
This sounds good. I can't offer any real feedback here. I don't work in the industry and don't know what real users would expect or how other libraries behave in this area. |
I've opened #70 to make this a little easier. I'm also considering adding a method which force closes a // Close will duplicate the first point as the last point
// for any ring which is currently open and has 3+ points.
func (r Ring) Close() {
if !r.Closed() && len(r) >= 3 {
r = append(r, r[0])
}
} I don't really like how similar |
According to geojson specification, polygon can contain closed Linear rings with 4 or more coordinates.
https://tools.ietf.org/html/rfc7946#section-3.1.6
However, the version 0.1.6 allows to create polygons with only 3 coordinates which is incosistent with the specification.
The text was updated successfully, but these errors were encountered: