polygons are inconsistent with geojson standard

[vohtaski]

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.

	poly := orb.Polygon{
		{
			{48.01300048828125, -21.32305807356671},
			{48.01300048828125, -21.32305807356671},
			{48.01300048828125, -21.32305807356671},
		},
	}
	fmt.Println(poly)

[paulmach]

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?

[vohtaski]

The issue I had was on clipping.
I clipped the polygon with orb/clip.
It produced the polygon above and javascript (turf.js) complained on the client.

Maybe clip library should validate?
I had to do the following cleanup to avoid the problem after polygon was clipped:

if geometry.GeoJSONType() == "Polygon" {
    // remove from polygons all rings with less than 4 coordinates
    // see: https://github.com/paulmach/orb/issues/45
    polygon := orb.Polygon{}
    for _, ring := range geometry.(orb.Polygon) {
        if len(ring) >= 4 {
            polygon = append(polygon, ring)
        }
    }
    if len(polygon) > 0 {
        feature.Geometry = polygon
        features.Append(feature)
    }
}

[antoniomo]

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 sortCCW() is defined as:

// 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()
}

[missinglink]

Heya, I found this issue when suffering similar issues yesterday.

It seems like there need to be some changes made to generate standard-compliant geojson polygons:

https://datatracker.ietf.org/doc/html/rfc7946#section-3.1.6

1. A linear ring is a closed LineString with four or more positions

for each ring in the polygon where ring.Closed() == false we need to duplicate the first point as the last point, if this is not possible or the result is len(ring) < 4 then warn and remove the invalid ring.

4. A linear ring MUST follow the right-hand rule with respect to the area it bounds, i.e., exterior rings are counterclockwise, and holes are clockwise

For Polygons with more than one of these rings, the first MUST be the exterior ring, and any others MUST be interior rings. The exterior ring bounds the surface, and the interior rings (if present) bound holes within the surface.

for each ring in the polygon where ring.Orientation() is not as expected we should call ring.Reverse(), presumably its CCW (1) for the first ring, then CW (-1) for subsequent rings. If it's 0 then I guess we warn and remove as above.

Regarding where in the codebase to implement this, I feel like there's two options:

1. the geojson functions mutate the original polygon
2. the geojson functions create a copy of the original polygon

... 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?

[missinglink]

There's also this Antimeridian Cutting rule and enforcing WGS84 but they are a task for another day 😆

[antoniomo]

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 .Validate() method to output an error or something.

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 .Standardize() method, prior to marshalling, to modify the polygon in-place. This would be backwards compatible and allow lower level hackery by bypassing validity when it's not wanted, but it's harder to use. What's better/more desirable?

[missinglink]

FWIW, I see github.com/paulmach/orb as a general purpose geo lib, and github.com/paulmach/orb/geojson as an extension for handling standards-compliant geojson.

As such I would put all the 'geojson stuff' in github.com/paulmach/orb/geojson and my vote would be for option 2, where the orb.Polygon is copied before mutating it in order to avoid forcing the general purpose lib to adopt geojsonisms.

This approach would work well for implementing other mutating features such as Antimeridian Cutting where IMO the original Polygon should be left unmodified but the geojson marshalling produces a MultiPolygon (not proposing we do that now BTW).

[antoniomo]

I can issue a PR to the geojson package within 1-2 weekends, as $DAYJOB has priority and I have some tight deadlines now 😄

[paulmach]

FWIW, I see github.com/paulmach/orb as a general purpose geo lib, and github.com/paulmach/orb/geojson as an extension for handling standards-compliant geojson.

As such I would put all the 'geojson stuff' in github.com/paulmach/orb/geojson and my vote would be for option 2, where the orb.Polygon is copied before mutating it in order to avoid forcing the general purpose lib to adopt geojsonisms.

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.

[missinglink]

I've opened #70 to make this a little easier.

I'm also considering adding a method which force closes a Ring but struggling to find the right API for it, maybe this?

// 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 Close() and Closed() bool are in their naming. Maybe ForceClosed(), but that seems to imply its forceful, which we can't guarantee for len(r) < 3 🤷‍♂️