Merge #356

356: Implements `Geometry::make_valid`. r=metasim a=metasim

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/gdal/blob/master/CODE_OF_CONDUCT.md).
- [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---

Partially addresses #306.

cc: `@thomas001`

Co-authored-by: Simeon H.K. Fitch <fitch@astraea.io>

CHANGES.md

@@ -26,6 +26,10 @@
 
   - <https://github.com/georust/gdal/pull/355>
 
+- Exposed various functions on `Geometry`:  `make_valid`, `geometry_name`, and `point_count`. 
+
+  - <https://github.com/georust/gdal/pull/356>
+
 ## 0.14
 
 - Added new content to `README.md` and the root docs.

src/cpl.rs

@@ -7,20 +7,23 @@ use std::ffi::CString;
 use std::fmt::{Debug, Formatter};
 use std::ptr;
 
-use gdal_sys::{CSLCount, CSLDestroy, CSLFetchNameValue, CSLSetNameValue};
+use gdal_sys::{CSLCount, CSLDestroy, CSLDuplicate, CSLFetchNameValue, CSLSetNameValue};
 use libc::c_char;
 
 use crate::errors::{GdalError, Result};
 use crate::utils::{_string, _string_tuple};
 
-/// Wraps a [`gdal_sys::CSLConstList`]  (a.k.a. `char **papszStrList`)
+/// Wraps a [`gdal_sys::CSLConstList`]  (a.k.a. `char **papszStrList`). This data structure
+/// (a null-terminated array of null-terminated strings) is used throughout GDAL to pass
+/// `KEY=VALUE`-formatted options to various functions.
 ///
 /// See the [`CSL*` GDAL functions](https://gdal.org/api/cpl.html#cpl-string-h) for more details.
 pub struct CslStringList {
     list_ptr: *mut *mut c_char,
 }
 
 impl CslStringList {
+    /// Creates an empty GDAL string list.
     pub fn new() -> Self {
         Self {
             list_ptr: ptr::null_mut(),
@@ -104,6 +107,14 @@ impl Default for CslStringList {
     }
 }
 
+impl Clone for CslStringList {
+    fn clone(&self) -> Self {
+        let list_ptr = unsafe { CSLDuplicate(self.list_ptr) };
+        Self { list_ptr }
+    }
+}
+
+/// State for iterator over [`CslStringList`] entries.
 pub struct CslStringListIterator<'a> {
     list: &'a CslStringList,
     idx: usize,
@@ -155,6 +166,47 @@ impl Debug for CslStringList {
     }
 }
 
+/// Convenience shorthand for specifying an empty `CslStringList` to functions accepting
+/// `Into<CslStringList>`.
+///
+/// # Example
+///
+/// ```rust, no_run
+/// use gdal::cpl::CslStringList;
+/// fn count_opts<O: Into<CslStringList>>(opts: O) -> usize {
+///     opts.into().len()
+/// }
+///
+/// assert_eq!(count_opts(()), 0);
+/// ```
+impl From<()> for CslStringList {
+    fn from(_: ()) -> Self {
+        CslStringList::default()
+    }
+}
+
+/// Convenience for creating a [`CslStringList`] from a slice of _key_/_value_ tuples.
+///
+/// # Example
+///
+/// ```rust, no_run
+/// use gdal::cpl::CslStringList;
+/// fn count_opts<O: Into<CslStringList>>(opts: O) -> usize {
+///     opts.into().len()
+/// }
+///
+/// assert_eq!(count_opts(&[("One", "1"), ("Two", "2"), ("Three", "3")]), 3);
+/// ```
+impl<const N: usize> From<&[(&str, &str); N]> for CslStringList {
+    fn from(pairs: &[(&str, &str); N]) -> Self {
+        let mut result = Self::default();
+        for (k, v) in pairs {
+            result.set_name_value(k, v).expect("valid key/value pair");
+        }
+        result
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use crate::cpl::CslStringList;

src/metadata.rs

@@ -235,7 +235,7 @@ pub trait Metadata: MajorObject {
     }
 }
 
-/// Standalone metadata entry, as returned by iterator from [`Metadata::metadata_iter`].
+/// Standalone metadata entry, as returned by iterator from [`Metadata::metadata`].
 ///
 /// Defined by it's parent `domain`, and `key`/`value` pair.
 #[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd)]

src/test_utils.rs

@@ -1,3 +1,5 @@
+use std::ffi::c_void;
+use std::marker::PhantomData;
 use std::path::{Path, PathBuf};
 
 /// A struct that contains a temporary directory and a path to a file in that directory.
@@ -49,3 +51,27 @@ pub(crate) fn fixture(filename: &str) -> PathBuf {
         .join("fixtures")
         .join(filename)
 }
+
+/// Scoped value for temporarily suppressing thread-local GDAL log messages.
+///
+/// Useful for tests that expect GDAL errors and want to keep the output log clean
+/// of distracting yet expected error messages.
+pub(crate) struct SuppressGDALErrorLog {
+    // Make !Sync and !Send, and force use of `new`.
+    _private: PhantomData<*mut c_void>,
+}
+
+impl SuppressGDALErrorLog {
+    pub(crate) fn new() -> Self {
+        unsafe { gdal_sys::CPLPushErrorHandler(Some(gdal_sys::CPLQuietErrorHandler)) };
+        SuppressGDALErrorLog {
+            _private: PhantomData,
+        }
+    }
+}
+
+impl Drop for SuppressGDALErrorLog {
+    fn drop(&mut self) {
+        unsafe { gdal_sys::CPLPopErrorHandler() };
+    }
+}

src/vector/geometry.rs

@@ -7,6 +7,7 @@ use std::ptr::null_mut;
 
 use libc::{c_char, c_double, c_int, c_void};
 
+use crate::cpl::CslStringList;
 use gdal_sys::{self, OGRErr, OGRGeometryH, OGRwkbGeometryType};
 
 use crate::errors::*;
@@ -224,11 +225,16 @@ impl Geometry {
         unsafe { gdal_sys::OGR_G_AddPoint_2D(self.c_geometry(), x as c_double, y as c_double) };
     }
 
-    pub fn get_point(&self, i: i32) -> (f64, f64, f64) {
+    /// Get point coordinates from a line string or a point geometry.
+    ///
+    /// `index` is the line string vertex index, from 0 to `point_count()-1`, or `0` when a point.
+    ///
+    /// See: [`OGR_G_GetPoint`](https://gdal.org/api/vector_c_api.html#_CPPv414OGR_G_GetPoint12OGRGeometryHiPdPdPd)
+    pub fn get_point(&self, index: i32) -> (f64, f64, f64) {
         let mut x: c_double = 0.;
         let mut y: c_double = 0.;
         let mut z: c_double = 0.;
-        unsafe { gdal_sys::OGR_G_GetPoint(self.c_geometry(), i, &mut x, &mut y, &mut z) };
+        unsafe { gdal_sys::OGR_G_GetPoint(self.c_geometry(), index, &mut x, &mut y, &mut z) };
         (x, y, z)
     }
 
@@ -292,15 +298,50 @@ impl Geometry {
         Ok(unsafe { Geometry::with_c_geometry(c_geom, true) })
     }
 
+    /// Get the geometry type ordinal
+    ///
+    /// See: [OGR_G_GetGeometryType](https://gdal.org/api/vector_c_api.html#_CPPv421OGR_G_GetGeometryType12OGRGeometryH)
     pub fn geometry_type(&self) -> OGRwkbGeometryType::Type {
         unsafe { gdal_sys::OGR_G_GetGeometryType(self.c_geometry()) }
     }
 
+    /// Get the WKT name for the type of this geometry.
+    ///
+    /// See: [`OGR_G_GetGeometryName`](https://gdal.org/api/vector_c_api.html#_CPPv421OGR_G_GetGeometryName12OGRGeometryH)
+    pub fn geometry_name(&self) -> String {
+        // Note: C API makes no statements about this possibly returning null.
+        // So we don't have to result wrap this,
+        let c_str = unsafe { gdal_sys::OGR_G_GetGeometryName(self.c_geometry()) };
+        if c_str.is_null() {
+            "".into()
+        } else {
+            _string(c_str)
+        }
+    }
+
+    /// Get the number of elements in a geometry, or number of geometries in container.
+    ///
+    /// Only geometries of type `wkbPolygon`, `wkbMultiPoint`, `wkbMultiLineString`, `wkbMultiPolygon`
+    /// or `wkbGeometryCollection` may return a non-zero value. Other geometry types will return 0.
+    ///
+    /// For a polygon, the returned number is the number of rings (exterior ring + interior rings).
+    ///
+    /// See: [`OGR_G_GetGeometryCount`](https://gdal.org/api/vector_c_api.html#_CPPv422OGR_G_GetGeometryCount12OGRGeometryH)
     pub fn geometry_count(&self) -> usize {
         let cnt = unsafe { gdal_sys::OGR_G_GetGeometryCount(self.c_geometry()) };
         cnt as usize
     }
 
+    /// Get the number of points from a Point or a LineString/LinearRing geometry.
+    ///
+    /// Only `wkbPoint` or `wkbLineString` may return a non-zero value. Other geometry types will return 0.
+    ///
+    /// See: [`OGR_G_GetPointCount`](https://gdal.org/api/vector_c_api.html#_CPPv419OGR_G_GetPointCount12OGRGeometryH)
+    pub fn point_count(&self) -> usize {
+        let cnt = unsafe { gdal_sys::OGR_G_GetPointCount(self.c_geometry()) };
+        cnt as usize
+    }
+
     /// Returns the n-th sub-geometry as a non-owned Geometry.
     ///
     /// # Safety
@@ -388,21 +429,18 @@ impl Geometry {
         unsafe { gdal_sys::OGR_G_Area(self.c_geometry()) }
     }
 
-    /// May or may not contain a reference to a SpatialRef: if not, it returns
-    /// an `Ok(None)`; if it does, it tries to build a SpatialRef. If that
-    /// succeeds, it returns an Ok(Some(SpatialRef)), otherwise, you get the
-    /// Err.
+    /// Get the spatial reference system for this geometry.
     ///
+    /// Returns `Some(SpatialRef)`, or `None` if one isn't defined.
+    ///
+    /// Refer: [OGR_G_GetSpatialReference](https://gdal.org/doxygen/ogr__api_8h.html#abc393e40282eec3801fb4a4abc9e25bf)
     pub fn spatial_ref(&self) -> Option<SpatialRef> {
         let c_spatial_ref = unsafe { gdal_sys::OGR_G_GetSpatialReference(self.c_geometry()) };
 
         if c_spatial_ref.is_null() {
             None
         } else {
-            match unsafe { SpatialRef::from_c_obj(c_spatial_ref) } {
-                Ok(sr) => Some(sr),
-                Err(_) => None,
-            }
+            unsafe { SpatialRef::from_c_obj(c_spatial_ref) }.ok()
         }
     }
 
@@ -416,6 +454,61 @@ impl Geometry {
     pub fn to_geo(&self) -> Result<geo_types::Geometry<f64>> {
         self.try_into()
     }
+
+    /// Attempts to make an invalid geometry valid without losing vertices.
+    ///
+    /// Already-valid geometries are cloned without further intervention.
+    ///
+    /// Extended options are available via [`CslStringList`] if GDAL is built with GEOS >= 3.8.
+    /// They are defined as follows:
+    ///
+    /// * `METHOD=LINEWORK`: Combines all rings into a set of node-ed lines and then extracts
+    ///    valid polygons from that "linework".
+    /// * `METHOD=STRUCTURE`: First makes all rings valid, then merges shells and subtracts holes
+    ///    from shells to generate valid result. Assumes holes and shells are correctly categorized.
+    /// * `KEEP_COLLAPSED=YES/NO`. Only for `METHOD=STRUCTURE`.
+    ///   - `NO` (default):  Collapses are converted to empty geometries
+    ///   - `YES`: collapses are converted to a valid geometry of lower dimension
+    ///
+    /// When GEOS < 3.8, this method will return `Ok(self.clone())` if it is valid, or `Err` if not.
+    ///
+    /// See: [OGR_G_MakeValidEx](https://gdal.org/api/vector_c_api.html#_CPPv417OGR_G_MakeValidEx12OGRGeometryH12CSLConstList)
+    ///
+    /// # Example
+    /// ```rust, no_run
+    /// use gdal::vector::Geometry;
+    /// # fn main() -> gdal::errors::Result<()> {
+    /// let src = Geometry::from_wkt("POLYGON ((0 0, 10 10, 0 10, 10 0, 0 0))")?;
+    /// let dst = src.make_valid(())?;
+    /// assert_eq!("MULTIPOLYGON (((10 0, 0 0, 5 5, 10 0)),((10 10, 5 5, 0 10, 10 10)))", dst.wkt()?);
+    /// # Ok(())
+    /// # }
+    /// ```
+    pub fn make_valid<O: Into<CslStringList>>(&self, opts: O) -> Result<Geometry> {
+        let opts = opts.into();
+
+        fn inner(geom: &Geometry, opts: CslStringList) -> Result<Geometry> {
+            #[cfg(all(major_ge_3, minor_ge_4))]
+            let c_geom = unsafe { gdal_sys::OGR_G_MakeValidEx(geom.c_geometry(), opts.as_ptr()) };
+
+            #[cfg(not(all(major_ge_3, minor_ge_4)))]
+            let c_geom = {
+                if !opts.is_empty() {
+                    return Err(GdalError::BadArgument(
+                        "Options to make_valid require GDAL >= 3.4".into(),
+                    ));
+                }
+                unsafe { gdal_sys::OGR_G_MakeValid(geom.c_geometry()) }
+            };
+
+            if c_geom.is_null() {
+                Err(_last_null_pointer_err("OGR_G_MakeValid"))
+            } else {
+                Ok(unsafe { Geometry::with_c_geometry(c_geom, true) })
+            }
+        }
+        inner(self, opts)
+    }
 }
 
 impl Drop for Geometry {
@@ -482,13 +575,14 @@ impl Debug for GeometryRef<'_> {
 
 #[cfg(test)]
 mod tests {
+    use super::*;
     use crate::spatial_ref::SpatialRef;
-
-    use super::{geometry_type_to_name, Geometry};
+    use crate::test_utils::SuppressGDALErrorLog;
 
     #[test]
     #[allow(clippy::float_cmp)]
     pub fn test_area() {
+        let _nolog = SuppressGDALErrorLog::new();
         let geom = Geometry::empty(::gdal_sys::OGRwkbGeometryType::wkbMultiPolygon).unwrap();
         assert_eq!(geom.area(), 0.0);
 
@@ -593,4 +687,40 @@ mod tests {
         // We don't care what it returns when passed an invalid value, just that it doesn't crash.
         geometry_type_to_name(4372521);
     }
+
+    #[test]
+    /// Simple clone case.
+    pub fn test_make_valid_clone() {
+        let src = Geometry::from_wkt("POINT (0 0)").unwrap();
+        let dst = src.make_valid(());
+        assert!(dst.is_ok());
+    }
+
+    #[test]
+    /// Un-repairable geometry case
+    pub fn test_make_valid_invalid() {
+        let _nolog = SuppressGDALErrorLog::new();
+        let src = Geometry::from_wkt("LINESTRING (0 0)").unwrap();
+        let dst = src.make_valid(());
+        assert!(dst.is_err());
+    }
+
+    #[test]
+    /// Repairable case (self-intersecting)
+    pub fn test_make_valid_repairable() {
+        let src = Geometry::from_wkt("POLYGON ((0 0, 10 10, 0 10, 10 0, 0 0))").unwrap();
+        let dst = src.make_valid(());
+        assert!(dst.is_ok());
+    }
+
+    #[cfg(all(major_ge_3, minor_ge_4))]
+    #[test]
+    /// Repairable case, but use extended options
+    pub fn test_make_valid_ex() {
+        let src =
+            Geometry::from_wkt("POLYGON ((0 0, 0 10, 10 10, 10 0, 0 0),(5 5, 15 10, 15 0, 5 5))")
+                .unwrap();
+        let dst = src.make_valid(&[("STRUCTURE", "LINEWORK")]);
+        assert!(dst.is_ok(), "{dst:?}");
+    }
 }