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

Polygons and MultiPolygons should follow the right-hand rule #39

Open
januszm opened this issue Mar 14, 2018 · 8 comments · May be fixed by #47
Open

Polygons and MultiPolygons should follow the right-hand rule #39

januszm opened this issue Mar 14, 2018 · 8 comments · May be fixed by #47
Labels

Comments

@januszm
Copy link

januszm commented Mar 14, 2018

Since GeoJSON is now a formal specification https://tools.ietf.org/html/rfc7946#section-3.1.6 I believe that RGeo::GeoJSON.encode should return a valid GeoJSON result.

One thing is to detect if polygons are right-hand wound (I guess Ruby Vector[].cross_product() method from the matrix module could do, some inspirations: https://github.com/mapbox/geojsonhint/blob/master/lib/rhr.js).

Second thing is to properly rewind the polygons (this JS package does the job: https://github.com/mapbox/geojson-rewind).

Let me know if this issue was already discussed or if someone already has a solution for this problem.

@januszm
Copy link
Author

januszm commented Mar 14, 2018

# https://github.com/alexreisner/geocoder/blob/master/lib/geocoder/calculations.rb
def to_radians(*args)
  args = args.first if args.first.is_a?(Array)
  if args.size == 1
    args.first * (Math::PI / 180)
  else
    args.map{ |i| to_radians(i) }
  end
end

# https://github.com/mapbox/geojsonhint/blob/master/lib/rhr.js
def is_ring_clockwise(coordinates)
  area = 0
  return if coordinates.size < 3
  coordinates.each_slice(2) do |point_1, point_2|
    area += to_radians(point_2[0] - point_1[0]) * (2 + Math.sin(to_radians(point_1[1])) + Math.sin(to_radians(point_2[1])))
  end
  area >= 0
end

# For MULTIPOLYGON
> shapefile = RGeo::Shapefile::Reader.open(my_shape_file)
> record = shapefile.next
> coordinates = record.geometry.coordinates.first.first # MultiPolygon->Polygon => [[x1, y1], ...]
> is_ring_clockwise(coordinates)
 => false
> coordinates_reversed = record.geometry.coordinates.map { |multipoly| multipoly.map { |poly| poly.reverse } }.first.first
> is_ring_clockwise(coordinates_reversed)
 => true

Something along those lines, it has to work for both exterior and interior rings, for each Polygon inside a Multipolygon, just like the orient method in Python's shapely package: https://github.com/Toblerity/Shapely/blob/master/shapely/geometry/polygon.py#L388

@januszm
Copy link
Author

januszm commented Mar 15, 2018

It should be even easier with PostGIS 2.4+. With the postgis adapter, we should be able to offload all computations to PostGIS functions http://postgis.net/docs/ST_ForcePolygonCW.html

@teeparham
Copy link
Member

Thanks for opening this issue and sharing some possible solutions.

Here's what the spec says:

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

 Note: the [GJ2008] specification did not discuss linear ring winding
 order.  For backwards compatibility, parsers SHOULD NOT reject
 Polygons that do not follow the right-hand rule.

This gem can't use the PostGIS function (although that is helpful and a good way to check).

RGeo::Cartesian::Analysis.ring_direction returns the orientation of a linestring (clockwise or counter-clockwise): https://github.com/rgeo/rgeo/blob/master/lib/rgeo/cartesian/analysis.rb#L22

@teeparham
Copy link
Member

Maybe we could add an option (clean) to the initializer of RGeo::GeoJSON::Coder. If clean is true, then polygons are forced to be right-hand wound. The default for clean should be false, since it might be slow and the spec says that parsers SHOULD NOT reject Polygons that do not follow the right-hand rule.

@teeparham
Copy link
Member

OR we could add a polygon cleaning method to rgeo, and leave this gem unchanged.

@januszm
Copy link
Author

januszm commented Mar 15, 2018

Thanks for the hint about RGeo::Cartesian::Analysis.ring_direction, we could use that. And I agree about PostGIS functions, I just wanted to share that finding in my last comment.

I was thinking about adding an option, something like strict: true/false, or clean like you proposed (naming things ... :) ) .

I think that this is more GeoJSON gem's responsibility - formatting the GeoJSON output properly. Users might want to keep the polygons source untouched (eg. db).

On the other hand, I can imagine that, for performance reasons, most users will want to clean their polygons in the database anyway.

@ilvez
Copy link

ilvez commented Feb 6, 2019

Is there any follow-up to this issue? I searched RGeo repo and didn't find anything, maybe I missed it. I'm currently struggling whether this should be solved in RGeo or PostGIS side.

@BuonOmo BuonOmo added feature documentation Issues that could be solved by adding documentation and removed documentation Issues that could be solved by adding documentation labels Nov 3, 2020
@BuonOmo BuonOmo linked a pull request Nov 4, 2020 that will close this issue
@BuonOmo BuonOmo linked a pull request Nov 9, 2020 that will close this issue
@BuonOmo
Copy link
Member

BuonOmo commented Nov 9, 2020

@januszm I've added this feature in #47. Since we're a bit short on maintainers, I'd be glad that you be part of the reviewers, at least for the dedicated commit (the last one)!

@ilvez the follow-up is coming. I think this should only be part of rgeo-geojson, I don't see why we should add such a restriction to rgeo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants