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?

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 ?

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.

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.

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

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

@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 :)

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

@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