Add validity check per method and validity API (#276)

Co-authored-by: Keith Doggett <keith.doggett887@gmail.com>

.github/workflows/CI.yml

@@ -20,7 +20,8 @@ jobs:
           - jruby
         os:
           - ubuntu
-          - windows
+          # Windows users, feel free to open a PR :)
+          # - windows
           - macos
     runs-on: ${{ matrix.os }}-latest
     continue-on-error: ${{ matrix.ruby == 'head' || matrix.os == 'windows' || matrix.os == 'macos' }}
@@ -30,7 +31,7 @@ jobs:
       - uses: ruby/setup-ruby@v1
         with:
           ruby-version: ${{ matrix.ruby }}
-          bundler-cache: true
+          bundler-cache: false
       - name: Install Geos (Linux)
         if: matrix.os == 'ubuntu'
         run: |
@@ -41,7 +42,9 @@ jobs:
         run: brew install geos
       - name: Install Geos (Windows)
         if: matrix.os == 'windows'
-        run: exit 1 # FIXME
+        run: exit 1
+      - name: Bundle Install
+        run: bundle install
       - name: Test
         run: bundle exec rake
   RuboCop:

History.md

@@ -2,6 +2,7 @@
 
 * Change ProjectedLinearRing #is_simple? method to be uniform across geos versions #228
 * Improve large MultiPolygon creation performance (Quiwin) #251
+* Major changes to validity handling TODO: complete that part of the changelog
 
 ### 2.3.0 / 2021-04-16
 

doc/Examples.md

@@ -76,7 +76,7 @@ This guide shows examples of different operations that can be performed with RGe
 
 ## Creating a Factory
 
-The examples below will show different methods for creating a factory with certain properties. 
+The examples below will show different methods for creating a factory with certain properties.
 
 _Note that these are not exhaustive and you should refer to the docs for a complete list of options._
 
@@ -87,7 +87,7 @@ Create a 2D Cartesian factory using a GEOS integration, if available, otherwise
 ```ruby
 factory = RGeo::Cartesian.factory
 p factory
-#=> #<RGeo::Geos::CAPIFactory srid=0 bufres=1 flags=8> 
+#=> #<RGeo::Geos::CAPIFactory srid=0 bufres=1 flags=8>
 ```
 
 ### Ruby Cartesian Factory
@@ -97,7 +97,7 @@ Create a 2D Cartesian factory using a Ruby implementation.
 ```ruby
 factory = RGeo::Cartesian.simple_factory
 p factory
-#=> #<RGeo::Cartesian::Factory @has_z=false, @has_m=false, @proj4=nil, @coord_sys=nil, @srid=0, @lenient_assertions=false, @buffer_resolution=1>
+#=> #<RGeo::Cartesian::Factory @has_z=false, @has_m=false, @proj4=nil, @coord_sys=nil, @srid=0, @buffer_resolution=1>
 ```
 
 ### Spherical Factory
@@ -164,7 +164,7 @@ p factory
 
 simple_factory = RGeo::Cartesian.simple_factory(has_z_coordinate: true, has_m_coordinate: true)
 p simple_factory
-#=> #<RGeo::Cartesian::Factory @has_z=true, @has_m=true, @proj4=nil, @coord_sys=nil, @srid=0, @lenient_assertions=false, @buffer_resolution=1>
+#=> #<RGeo::Cartesian::Factory @has_z=true, @has_m=true, @proj4=nil, @coord_sys=nil, @srid=0, @buffer_resolution=1>
 ```
 
 ### Specify an SRID
@@ -179,19 +179,6 @@ p factory
 #=> #<RGeo::Geos::CAPIFactory:0x2ad702f454bc srid=3857 bufres=1 flags=8>
 ```
 
-### Factory that Allows Invalid Geometries
-
-By default, most factories will not allow the creation of invalid geometries and will raise an `RGeo::Error::InvalidGeometry` exception. This can be set with the `:uses_lenient_assertions` parameter.
-
-```ruby
-factory = RGeo::Geographic.spherical_factory(uses_lenient_assertions: true)
-p factory
-#=> #<RGeo::Geographic::Factory:0x000055ae05d98150 @impl_prefix="Spherical", @point_class=RGeo::Geographic::SphericalPointImpl, @line_string_class=RGeo::Geographic::SphericalLineStringImpl, @linear_ring_class=RGeo::Geographic::SphericalLinearRingImpl, @line_class=RGeo::Geographic::SphericalLineImpl, @polygon_class=RGeo::Geographic::SphericalPolygonImpl, @geometry_collection_class=RGeo::Geographic::SphericalGeometryCollectionImpl, @multi_point_class=RGeo::Geographic::SphericalMultiPointImpl, @multi_line_string_class=RGeo::Geographic::SphericalMultiLineStringImpl, @multi_polygon_class=RGeo::Geographic::SphericalMultiPolygonImpl, @support_z=false, @support_m=false, @srid=4055, @proj4=nil>
-
-p factory.property(:uses_lenient_assertions)
-#=> true
-```
-
 ## Points
 
 The zero-dimensional object which all geometries are built on. Points accept floating-point numbers as arguments for x,y(,z,m).
@@ -257,7 +244,7 @@ pt2 = factory.point(2, 0)
 pt3 = factory.point(2, 2)
 linestring = factory.line_string([pt1, pt2, pt3])
 p linestring
-# => #<RGeo::Geos::CAPILineStringImpl "LINESTRING (0.0 0.0, 2.0 0.0, 2.0 2.0)"> 
+# => #<RGeo::Geos::CAPILineStringImpl "LINESTRING (0.0 0.0, 2.0 0.0, 2.0 2.0)">
 ```
 
 
@@ -271,7 +258,7 @@ This creates the same LineString as above.
 factory = RGeo::Cartesian.factory
 linestring2 = factory.parse_wkt("LINESTRING (0 0, 2 0, 2 2)")
 p linestring2
-# => #<RGeo::Geos::CAPILineStringImpl "LINESTRING (0.0 0.0, 2.0 0.0, 2.0 2.0)"> 
+# => #<RGeo::Geos::CAPILineStringImpl "LINESTRING (0.0 0.0, 2.0 0.0, 2.0 2.0)">
 # p linestring == linestring2
 # => true
 ```
@@ -317,7 +304,7 @@ pt3 = factory.point(4, 4)
 pt4 = factory.point(2, 6)
 linearring = factory.linear_ring([pt1, pt2, pt3, pt4, pt1])
 p linearring
-# => #<RGeo::Geos::CAPILinearRingImpl "LINESTRING (2.0 2.0, 4.0 2.0, 4.0 4.0, 2.0 6.0, 2.0 2.0)"> 
+# => #<RGeo::Geos::CAPILinearRingImpl "LINESTRING (2.0 2.0, 4.0 2.0, 4.0 4.0, 2.0 6.0, 2.0 2.0)">
 p linearring.is_closed?
 #=> true
 p linearring.is_simple?
@@ -731,7 +718,7 @@ Get the WKT representation of a geometry.
 ```ruby
 factory = RGeo::Cartesian.factory
 p factory.point(1, 1).as_text
-# => "POINT (1.0 1.0)" 
+# => "POINT (1.0 1.0)"
 ```
 
 ### as_binary
@@ -741,7 +728,7 @@ Get the WKB representation of a geometry.
 ```ruby
 factory = RGeo::Cartesian.factory
 p factory.point(1, 1).as_binary
-# => "\x00\x00\x00\x00\x01?\xF0\x00\x00\x00\x00\x00\x00?\xF0\x00\x00\x00\x00\x00\x00" 
+# => "\x00\x00\x00\x00\x01?\xF0\x00\x00\x00\x00\x00\x00?\xF0\x00\x00\x00\x00\x00\x00"
 ```
 
 ## Predicates and Relationships
@@ -1048,7 +1035,7 @@ square2 = factory.parse_wkt("POLYGON ((1 0, 1 2, 3 2, 3 0, 1 0))")
 intersection = square1.intersection(square2)
 
 p intersection
-# => #<RGeo::Geos::CAPIPolygonImpl "POLYGON ((1.0 2.0, 2.0 2.0, 2.0 0.0, 1.0 0.0, 1.0 2.0))"> 
+# => #<RGeo::Geos::CAPIPolygonImpl "POLYGON ((1.0 2.0, 2.0 2.0, 2.0 0.0, 1.0 0.0, 1.0 2.0))">
 ```
 
 ### Union
@@ -1079,7 +1066,7 @@ squares = factory.collection([square1, square2])
 union = squares.unary_union
 
 p union
-# => #<RGeo::Geos::CAPIPolygonImpl "POLYGON ((0.0 0.0, 0.0 2.0, 1.0 2.0, 2.0 2.0, 3.0 2.0, 3.0 0.0, 2.0 0.0, 1.0 0.0, 0.0 0.0))"> 
+# => #<RGeo::Geos::CAPIPolygonImpl "POLYGON ((0.0 0.0, 0.0 2.0, 1.0 2.0, 2.0 2.0, 3.0 2.0, 3.0 0.0, 2.0 0.0, 1.0 0.0, 0.0 0.0))">
 ```
 
 ### Difference
@@ -1094,7 +1081,7 @@ square2 = factory.parse_wkt("POLYGON ((1 0, 1 2, 3 2, 3 0, 1 0))")
 diff = square1.difference(square2)
 
 p diff
-# => #<RGeo::Geos::CAPIPolygonImpl "POLYGON ((0.0 0.0, 0.0 2.0, 1.0 2.0, 1.0 0.0, 0.0 0.0))"> 
+# => #<RGeo::Geos::CAPIPolygonImpl "POLYGON ((0.0 0.0, 0.0 2.0, 1.0 2.0, 1.0 0.0, 0.0 0.0))">
 ```
 
 ### SymDifference
@@ -1110,4 +1097,4 @@ sdiff = square1.sym_difference(square2)
 
 p sdiff
 # => #<RGeo::Geos::CAPIMultiPolygonImpl "MULTIPOLYGON (((0.0 0.0, 0.0 2.0, 1.0 2.0, 1.0 0.0, 0.0 0.0)), ((2.0 0.0, 2.0 2.0, 3.0 2.0, 3.0 0.0, 2.0 0.0)))">
-```
+```

doc/Factory-Compatibility.md

@@ -318,6 +318,7 @@ _Note: This list is not exhaustive of all the methods defined by each factory. T
 |                             `Polygon#boundary` |  ✅   |    ✅    |  ✅  |   ✅    |     ✅     |     ✅      |     ✅     |
 |                          `Polygon#convex_hull` |  ✅   |    ✅    |  ✅  |   ✅    |     ❌     |     ✅      |     ❌     |
 |                               `Polygon#buffer` |  ✅   |    ✅    |  ✅  |   ✅    |     ❌     |     ✅      |     ❌     |
+|                                 `Polygon#area` |  ✅   |    ✅    |  ✅  |   ✅    |     ❌     |     ✅      |     ❌     |
 |                       `Polygon#equals?(Point)` |  ✅   |    ✅    |  ✅  |   ✅    |     ❌     |     ✅      |     ❌     |
 |                  `Polygon#equals?(LineString)` |  ✅   |    ✅    |  ✅  |   ✅    |     ❌     |     ✅      |     ❌     |
 |                  `Polygon#equals?(LinearRing)` |  ✅   |    ✅    |  ✅  |   ✅    |     ❌     |     ✅      |     ❌     |
@@ -686,6 +687,7 @@ _Note: This list is not exhaustive of all the methods defined by each factory. T
 |                        `MultiPolygon#boundary` |  ✅   |    ✅    |  ✅  |   ✅    |     ✅     |     ✅      |     ✅     |
 |                     `MultiPolygon#convex_hull` |  ✅   |    ✅    |  ✅  |   ✅    |     ❌     |     ✅      |     ❌     |
 |                          `MultiPolygon#buffer` |  ✅   |    ✅    |  ✅  |   ✅    |     ❌     |     ✅      |     ❌     |
+|                            `MultiPolygon#area` |  ✅   |    ✅    |  ✅  |   ✅    |     ❌     |     ✅      |     ❌     |
 |                  `MultiPolygon#equals?(Point)` |  ✅   |    ✅    |  ✅  |   ✅    |     ❌     |     ✅      |     ❌     |
 |             `MultiPolygon#equals?(LineString)` |  ✅   |    ✅    |  ✅  |   ✅    |     ❌     |     ✅      |     ❌     |
 |             `MultiPolygon#equals?(LinearRing)` |  ✅   |    ✅    |  ✅  |   ✅    |     ❌     |     ✅      |     ❌     |

doc/Which-factory-should-I-use.md

@@ -65,17 +65,15 @@ Once you've selected what type of factory you want from the list above, you need
 
 `has_m_coordinate` -- Gives all geometries created with the factory an M-coordinate.
 
-`uses_lenient_assertions`: Specify that invalid geometries may be instantiated if set to `true`.
-
 See the documentation for a complete list for each factory.
 
 Now you can create your factory:
 
 ```rb
 require 'rgeo'
 
-# Create a 3D Cartesian factory that allows for invalid geometries to be created. Specify that geometries exist in a mercator project (srid: 3857).
-factory = RGeo::Cartesian.preferred_factory(uses_lenient_assertions: true, has_z_coordinate: true, srid: 3857)
+# Create a 3D Cartesian factory. Specify that geometries exist in a mercator project (srid: 3857).
+factory = RGeo::Cartesian.preferred_factory(has_z_coordinate: true, srid: 3857)
 ```
 
 To create geometries, using the factory design pattern is quite straightforward: just do never instantiate your objects with `new`, but rather use your factory to create an object.

ext/geos_c_impl/factory.c

@@ -167,6 +167,25 @@ static VALUE method_factory_flags(VALUE self)
   return INT2NUM(RGEO_FACTORY_DATA_PTR(self)->flags);
 }
 
+VALUE method_factory_supports_z_p(VALUE self)
+{
+  return RGEO_FACTORY_DATA_PTR(self)->flags & RGEO_FACTORYFLAGS_SUPPORTS_Z ? Qtrue : Qfalse;
+}
+
+VALUE method_factory_supports_m_p(VALUE self)
+{
+  return RGEO_FACTORY_DATA_PTR(self)->flags & RGEO_FACTORYFLAGS_SUPPORTS_M ? Qtrue : Qfalse;
+}
+
+VALUE method_factory_supports_z_or_m_p(VALUE self)
+{
+  return RGEO_FACTORY_DATA_PTR(self)->flags & RGEO_FACTORYFLAGS_SUPPORTS_Z_OR_M ? Qtrue : Qfalse;
+}
+
+VALUE method_factory_prepare_heuristic_p(VALUE self)
+{
+  return RGEO_FACTORY_DATA_PTR(self)->flags & RGEO_FACTORYFLAGS_PREPARE_HEURISTIC ? Qtrue : Qfalse;
+}
 
 static VALUE method_factory_parse_wkt(VALUE self, VALUE str)
 {
@@ -609,15 +628,26 @@ RGeo_Globals* rgeo_init_geos_factory()
   globals->marshal_wkb_generator = Qnil;
 #endif
 
-  // Add C methods to the factory.
   geos_factory_class = rb_define_class_under(globals->geos_module, "CAPIFactory", rb_cObject);
   rb_define_alloc_func(geos_factory_class, alloc_factory);
+  // Add C constants to the factory.
+  rb_define_const(geos_factory_class, "FLAG_LENIENT_MULTIPOLYGON_ASSERTIONS", INT2FIX(RGEO_FACTORYFLAGS_LENIENT_MULTIPOLYGON_ASSERTIONS));
+  rb_define_const(geos_factory_class, "FLAG_SUPPORTS_Z", INT2FIX(RGEO_FACTORYFLAGS_SUPPORTS_Z));
+  rb_define_const(geos_factory_class, "FLAG_SUPPORTS_M", INT2FIX(RGEO_FACTORYFLAGS_SUPPORTS_M));
+  rb_define_const(geos_factory_class, "FLAG_SUPPORTS_Z_OR_M", INT2FIX(RGEO_FACTORYFLAGS_SUPPORTS_Z_OR_M));
+  rb_define_const(geos_factory_class, "FLAG_PREPARE_HEURISTIC", INT2FIX(RGEO_FACTORYFLAGS_PREPARE_HEURISTIC));
+  rb_define_const(geos_factory_class, "FLAG_LENIENT_ASSERTIONS", INT2FIX(RGEO_FACTORYFLAGS_LENIENT_ASSERTIONS));
+  // Add C methods to the factory.
   rb_define_method(geos_factory_class, "initialize_copy", method_factory_initialize_copy, 1);
   rb_define_method(geos_factory_class, "_parse_wkt_impl", method_factory_parse_wkt, 1);
   rb_define_method(geos_factory_class, "_parse_wkb_impl", method_factory_parse_wkb, 1);
   rb_define_method(geos_factory_class, "_srid", method_factory_srid, 0);
   rb_define_method(geos_factory_class, "_buffer_resolution", method_factory_buffer_resolution, 0);
   rb_define_method(geos_factory_class, "_flags", method_factory_flags, 0);
+  rb_define_method(geos_factory_class, "supports_z?", method_factory_supports_z_p, 0);
+  rb_define_method(geos_factory_class, "supports_m?", method_factory_supports_m_p, 0);
+  rb_define_method(geos_factory_class, "supports_z_or_m?", method_factory_supports_z_or_m_p, 0);
+  rb_define_method(geos_factory_class, "prepare_heuristic?", method_factory_prepare_heuristic_p, 0);
   rb_define_method(geos_factory_class, "_set_wkrep_parsers", method_set_wkrep_parsers, 2);
   rb_define_method(geos_factory_class, "_proj4", method_get_proj4, 0);
   rb_define_method(geos_factory_class, "_coord_sys", method_get_coord_sys, 0);

ext/geos_c_impl/factory.h

@@ -82,12 +82,37 @@ typedef struct {
   int buffer_resolution;
 } RGeo_FactoryData;
 
-#define RGEO_FACTORYFLAGS_LENIENT_MULTIPOLYGON 1
-#define RGEO_FACTORYFLAGS_SUPPORTS_Z 2
-#define RGEO_FACTORYFLAGS_SUPPORTS_M 4
-#define RGEO_FACTORYFLAGS_SUPPORTS_Z_OR_M 6
-#define RGEO_FACTORYFLAGS_PREPARE_HEURISTIC 8
-
+/*
+  Flags that are used to pass options when creating a factory.
+  They are available in ruby under RGeo::Geos::CAPIFactory::FLAG_name
+  where name is the name below without the RGEO_FACTORYFLAGS_ prefix.
+ */
+#define RGEO_FACTORYFLAGS_LENIENT_MULTIPOLYGON_ASSERTIONS 0b00001
+#define RGEO_FACTORYFLAGS_SUPPORTS_Z                      0b00010
+#define RGEO_FACTORYFLAGS_SUPPORTS_M                      0b00100
+#define RGEO_FACTORYFLAGS_SUPPORTS_Z_OR_M                 (RGEO_FACTORYFLAGS_SUPPORTS_Z | RGEO_FACTORYFLAGS_SUPPORTS_M)
+#define RGEO_FACTORYFLAGS_PREPARE_HEURISTIC               0b01000
+#define RGEO_FACTORYFLAGS_LENIENT_ASSERTIONS              0b10000
+
+/* call-seq:
+ *   RGeo::Geos::CAPIFactory.supports_z? -> true or false
+ */
+VALUE method_factory_supports_z_p(VALUE self);
+
+/* call-seq:
+ *   RGeo::Geos::CAPIFactory.supports_m? -> true or false
+ */
+VALUE method_factory_supports_m_p(VALUE self);
+
+/* call-seq:
+ *   RGeo::Geos::CAPIFactory.supports_z_or_m? -> true or false
+ */
+VALUE method_factory_supports_z_or_m_p(VALUE self);
+
+/* call-seq:
+ *   RGeo::Geos::CAPIFactory.prepare_heuristic? -> true or false
+ */
+VALUE method_factory_prepare_heuristic_p(VALUE self);
 
 /*
   Wrapped structure for Geometry objects.

ext/geos_c_impl/geometry.c

@@ -1044,18 +1044,38 @@ static VALUE method_geometry_invalid_reason(VALUE self)
   self_data = RGEO_GEOMETRY_DATA_PTR(self);
   self_geom = self_data->geom;
   if (self_geom) {
-    str = GEOSisValidReason_r(self_data->geos_context, self_geom);
-    // Per documentation, a valid geometry should give an empty string.
-    // However it seems not to be the case. Hence the comparison against
-    // the string that is really given: `"Valid Geometry"`.
-    // See https://github.com/libgeos/geos/issues/431.
-    if (str) result = (str[0] == '\0' || !strcmp(str, "Valid Geometry")) ? Qnil : rb_str_new2(str);
-    else result = rb_str_new2("Exception");
-    GEOSFree_r(self_data->geos_context, str);
+    // TODO: should we consider using the flag GEOSVALID_ALLOW_SELFTOUCHING_RING_FORMING_HOLE?
+
+    // We use NULL there to tell GEOS that we don't care about the position.
+    switch(GEOSisValidDetail_r(self_data->geos_context, self_geom, 0, &str, NULL)) {
+      case 0: // invalid
+        result = rb_utf8_str_new_cstr(str);
+      case 1: // valid
+        break;
+      case 2: // exception
+      default:
+        result = rb_utf8_str_new_cstr("Exception");
+        break;
+    };
+    if (str) GEOSFree_r(self_data->geos_context, str);
   }
   return result;
 }
 
+static VALUE method_geometry_make_valid(VALUE self)
+{
+  RGeo_GeometryData* self_data;
+  const GEOSGeometry* self_geom;
+  GEOSGeometry* valid_geom;
+  self_data = RGEO_GEOMETRY_DATA_PTR(self);
+  self_geom = self_data->geom;
+  if (!self_geom) return Qnil;
+
+  // According to GEOS implementation, MakeValid always returns.
+  valid_geom = GEOSMakeValid_r(self_data->geos_context, self_geom);
+  return rgeo_wrap_geos_geometry(self_data->factory, valid_geom, Qnil);
+}
+
 static VALUE method_geometry_point_on_surface(VALUE self)
 {
   VALUE result;
@@ -1126,6 +1146,7 @@ void rgeo_init_geos_geometry(RGeo_Globals* globals)
   rb_define_method(geos_geometry_methods, "valid?", method_geometry_is_valid, 0);
   rb_define_method(geos_geometry_methods, "invalid_reason", method_geometry_invalid_reason, 0);
   rb_define_method(geos_geometry_methods, "point_on_surface", method_geometry_point_on_surface, 0);
+  rb_define_method(geos_geometry_methods, "make_valid", method_geometry_make_valid, 0);
 }
 
 

ext/geos_c_impl/geometry_collection.c

@@ -85,14 +85,6 @@ static VALUE create_geometry_collection(VALUE module, int type, VALUE factory, V
     }
     else {
       collection = GEOSGeom_createCollection_r(geos_context, type, geoms, len);
-      // Check if MultiPolygon is valid.
-      if (collection
-          && type == GEOS_MULTIPOLYGON
-          && !(factory_data->flags & 1)
-          && !GEOSisValid_r(geos_context, collection)) {
-        GEOSGeom_destroy_r(geos_context, collection);
-        collection = NULL;
-      }
       if (collection) {
         result = rgeo_wrap_geos_geometry(factory, collection, module);
         RGEO_GEOMETRY_DATA_PTR(result)->klasses = klasses;
@@ -375,7 +367,7 @@ static VALUE method_multi_line_string_coordinates(VALUE self)
 
   self_data = RGEO_GEOMETRY_DATA_PTR(self);
   self_geom = self_data->geom;
-  
+
   if(self_geom) {
     zCoordinate = RGEO_FACTORY_DATA_PTR(self_data->factory)->flags & RGEO_FACTORYFLAGS_SUPPORTS_Z_OR_M;
     context = self_data->geos_context;
@@ -487,7 +479,7 @@ static VALUE method_multi_polygon_coordinates(VALUE self)
 
   self_data = RGEO_GEOMETRY_DATA_PTR(self);
   self_geom = self_data->geom;
-  
+
   if(self_geom) {
     zCoordinate = RGEO_FACTORY_DATA_PTR(self_data->factory)->flags & RGEO_FACTORYFLAGS_SUPPORTS_Z_OR_M;
     context = self_data->geos_context;