Change for a more strict geojson handling.

[BuonOmo]

Done:

• Do not handle m coordinate.
• Raise for unknown geojson type.
• Add various tests.
• About Fail to decode polygons with overlapping areas #33, we may want to change the default factory for one having uses_lenient_assertions to allow complex polygons.
  => We could also fix the issue in RGeo altogether (https://github.com/rgeo/rgeo/compare/fix/correct-area-when-invalid-polygon). But this comes with a performance cost that we should avoid, at least until another major. Hence, and since anyway it seems like people expect those polygons to pass (which is also per spec), we let those pass in the mean time.

Question:

• About Polygons and MultiPolygons should follow the right-hand rule #39, shouldn't we force clockwise encoding when encoding a feature to respect specs, and not change decoding ? This would fit to specs:

  • 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.
  => A solution is implemented in the last commit. Maybe we should challenge it due to the performance issue

[keithdoggett]

Should there be a test for this?

[keithdoggett]

Looks good! Once that GeometryCollection test is resolved, it should be good to merge. We'll likely have to do a major version release because of the deprecation of m coordinates. Thoughts?

[BuonOmo]

That is definitely a major. Deprecation of, significant performance changes. raise instead of nil.

I'll

• complete the History.md,
• change the readme to address the new version,
• add the desired test.

And then give it back to you for a final review :)

About the performance drawback in b9bc47a, are you ok with that ?

[keithdoggett]

Since it's just on the encoder and the 2016 GeoJSON spec indicates that the winding must follow the right-hand rule I think it should be included. We can still parse GeoJSON that doesn't follow the right-hand rule so it shouldn't limit accesability.

If we're seeing that it's too much of a performance hit for it to be usable for a good amount of people we could make an option to enforce it later, but I think following the spec is important.

[keithdoggett]

Actually, after looking at this comment #39 (comment), we should probably add the option in this release if it's not too much work.

I think making it true by default and conforming to the standard by default is good, but I imagine a lot of people won't care much if it's for internal use only.

[BuonOmo]

Thanks for your inputs @keithdoggett,

Since I find it bothering to have a more complex API, I tried to find a way to improve performances.

Hence I looked at geos, and the is_ccw method in geos, which would be more efficient that our ring_direction ruby implementation. Here's the result of the benchmark after I've implemented it:

Without direction check:

Warming up --------------------------------------
        big geometry    23.000  i/100ms
      small geometry    12.024k i/100ms
Calculating -------------------------------------
        big geometry    227.621  (± 4.8%) i/s -      1.150k in   5.066217s
      small geometry    119.305k (± 2.9%) i/s -    601.200k in   5.043854s

With a ruby direction check:

Warming up --------------------------------------
        big geometry     1.000  i/100ms
      small geometry     2.913k i/100ms
Calculating -------------------------------------
        big geometry      8.871  (± 0.0%) i/s -     45.000  in   5.084801s
      small geometry     27.025k (± 8.6%) i/s -    133.998k in   5.003739s

With a c direction check:

Warming up --------------------------------------
        big geometry    24.000  i/100ms
      small geometry     8.902k i/100ms
Calculating -------------------------------------
        big geometry    213.825  (± 7.0%) i/s -      1.080k in   5.080621s
      small geometry     86.265k (± 5.0%) i/s -    436.198k in   5.072204s

We can see that performance is nearly the same between c implementation and no check. Hence we could not add the clean: option, and rather encourage people to use libgeos (which is the best way to use RGeo anyway!).

What do you think ? If you like this idea, I can push the c analysis implementation is rgeo as well, which is quite light anyway :) (this would still be a minor, since i'll add a method in the Analysis module.)

[keithdoggett]

Yes, I agree that keeping the API simple is best and that seems like a good solution!

[BuonOmo]

@keithdoggett once the rgeo/rgeo#229 is merged and published, I'll change the method name here and we'll be able to publish the new version :)

[keithdoggett]

@BuonOmo what's the status of this PR? Do we need to update it to use the new ccw? method. We should also make sure that if somebody isn't using rgeo-2.2.0, there is support for that.

[BuonOmo]

@keithdoggett thanks for pointing it out indeed.

I think we can:

• update the current code using ring.ccw? method instead of the clockwise?(ring) method
• specify that rgeo is required to be at least 2.2.0, since the PR has to be included in a major, I think we can afford this.

Does it look OK to you ?

I'm not sure I'll have time to do it this week though