From f39e73d83c3b084e633e9f86f434c2b073e0c31a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lauren=C8=9Biu=20Nicola?= Date: Sat, 11 Nov 2023 11:28:58 +0200 Subject: [PATCH 1/8] Drop lifetime in GdalDEMProcessingOptions --- src/raster/processing/dem/options.rs | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/raster/processing/dem/options.rs b/src/raster/processing/dem/options.rs index 76b33113..85b01abc 100644 --- a/src/raster/processing/dem/options.rs +++ b/src/raster/processing/dem/options.rs @@ -1,5 +1,4 @@ use std::fmt::{Display, Formatter}; -use std::marker::PhantomData; use std::ptr; use std::ptr::NonNull; @@ -12,18 +11,17 @@ use crate::errors; use crate::utils::_last_null_pointer_err; /// Payload for [`GDALDEMProcessing`]. Intended for internal use only. -pub struct GdalDEMProcessingOptions<'opts>( - NonNull, - PhantomData<&'opts CslStringList>, -); +pub struct GdalDEMProcessingOptions(NonNull); -impl<'opts> GdalDEMProcessingOptions<'opts> { - pub fn new(opts: &'opts CslStringList) -> errors::Result { +impl GdalDEMProcessingOptions { + pub fn new(opts: &CslStringList) -> errors::Result { + // GDAL copies the relevant value out of `opts`, we don't need to keep them alive: + // https://github.com/OSGeo/gdal/blob/59eaaed3168f49e8a7a3821730277aff68a86d16/apps/gdaldem_lib.cpp#L3770 let popts = unsafe { GDALDEMProcessingOptionsNew(opts.as_ptr(), ptr::null_mut()) }; - if popts.is_null() { - return Err(_last_null_pointer_err("GDALDEMProcessingOptionsNew")); + match NonNull::new(popts) { + Some(popts) => Ok(Self(popts)), + None => Err(_last_null_pointer_err("GDALDEMProcessingOptionsNew")), } - Ok(Self(unsafe { NonNull::new_unchecked(popts) }, PhantomData)) } pub fn as_ptr(&self) -> *const GDALDEMProcessingOptions { @@ -31,7 +29,7 @@ impl<'opts> GdalDEMProcessingOptions<'opts> { } } -impl Drop for GdalDEMProcessingOptions<'_> { +impl Drop for GdalDEMProcessingOptions { fn drop(&mut self) { unsafe { GDALDEMProcessingOptionsFree(self.0.as_ptr()) }; } From 25070f61e59c0eb89c5582cc3dac00424319541d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lauren=C8=9Biu=20Nicola?= Date: Sat, 11 Nov 2023 11:45:58 +0200 Subject: [PATCH 2/8] Don't rely on Debug impls --- src/raster/processing/dem/aspect.rs | 2 +- src/raster/processing/dem/hillshade.rs | 16 +++++++++------- src/raster/processing/dem/mod.rs | 4 ++-- src/raster/processing/dem/options.rs | 26 +++++++++++++++----------- src/raster/processing/dem/slope.rs | 2 +- src/raster/processing/dem/tri.rs | 13 ++++++++----- 6 files changed, 36 insertions(+), 27 deletions(-) diff --git a/src/raster/processing/dem/aspect.rs b/src/raster/processing/dem/aspect.rs index a787cc57..0f43e3eb 100644 --- a/src/raster/processing/dem/aspect.rs +++ b/src/raster/processing/dem/aspect.rs @@ -52,7 +52,7 @@ impl AspectOptions { if let Some(alg) = self.algorithm { opts.add_string("-alg").unwrap(); - opts.add_string(&alg.to_string()).unwrap(); + opts.add_string(alg.to_gdal_option()).unwrap(); } if self.zero_for_flat == Some(true) { diff --git a/src/raster/processing/dem/hillshade.rs b/src/raster/processing/dem/hillshade.rs index 44e771d9..b25a7758 100644 --- a/src/raster/processing/dem/hillshade.rs +++ b/src/raster/processing/dem/hillshade.rs @@ -1,7 +1,6 @@ use crate::cpl::CslStringList; use crate::raster::processing::dem::options::common_dem_options; use crate::raster::processing::dem::DemSlopeAlg; -use std::fmt::{Display, Formatter}; use std::num::NonZeroUsize; /// Configuration options for [`hillshade()`][super::hillshade()]. @@ -122,7 +121,7 @@ impl HillshadeOptions { if let Some(alg) = self.algorithm { opts.add_string("-alg").unwrap(); - opts.add_string(&alg.to_string()).unwrap(); + opts.add_string(alg.to_gdal_option()).unwrap(); } if let Some(scale) = self.scale { @@ -131,7 +130,7 @@ impl HillshadeOptions { } if let Some(mode) = self.shading { - opts.add_string(&format!("-{mode}")).unwrap(); + opts.add_string(mode.to_gdal_option()).unwrap(); } if let Some(factor) = self.z_factor { @@ -172,10 +171,13 @@ pub enum ShadingMode { Igor, } -impl Display for ShadingMode { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - let s = format!("{self:?}"); - f.write_str(&s.to_lowercase()) +impl ShadingMode { + pub(crate) fn to_gdal_option(&self) -> &'static str { + match self { + ShadingMode::Combined => "-combined", + ShadingMode::Multidirectional => "-multidirectional", + ShadingMode::Igor => "-igor", + } } } diff --git a/src/raster/processing/dem/mod.rs b/src/raster/processing/dem/mod.rs index 12bdb199..c817856f 100644 --- a/src/raster/processing/dem/mod.rs +++ b/src/raster/processing/dem/mod.rs @@ -372,7 +372,7 @@ pub fn slope>( /// std_dev: 0.48943078832474257, /// } /// ``` -/// +/// /// See: [`gdaldem tpi`](https://gdal.org/programs/gdaldem.html#tpi) for details. pub fn topographic_position_index>( ds: &Dataset, @@ -447,7 +447,7 @@ fn dem_eval( color_relief_config: Option, ) -> Result { let popts = options::GdalDEMProcessingOptions::new(options)?; - let mode = CString::new(alg.to_string())?; + let mode = CString::new(alg.to_gdal_option())?; let dest = _path_to_c_string(dst_file)?; let cfile = color_relief_config.and_then(|p| _path_to_c_string(&p).ok()); let cfile_ptr = cfile.as_deref().map(CStr::as_ptr).unwrap_or(ptr::null()); diff --git a/src/raster/processing/dem/options.rs b/src/raster/processing/dem/options.rs index 85b01abc..0b8e59a9 100644 --- a/src/raster/processing/dem/options.rs +++ b/src/raster/processing/dem/options.rs @@ -1,4 +1,3 @@ -use std::fmt::{Display, Formatter}; use std::ptr; use std::ptr::NonNull; @@ -47,14 +46,16 @@ pub enum DemAlg { Tri, } -impl Display for DemAlg { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { +impl DemAlg { + pub(crate) fn to_gdal_option(&self) -> &'static str { match self { - Self::ColorRelief => f.write_str("color-relief"), - _ => { - let s = format!("{self:?}").to_lowercase(); - f.write_str(&s) - } + DemAlg::Aspect => "aspect", + DemAlg::ColorRelief => "color-relief", + DemAlg::Hillshade => "hillshade", + DemAlg::Roughness => "roughness", + DemAlg::Slope => "slope", + DemAlg::Tpi => "TPI", + DemAlg::Tri => "TRI", } } } @@ -69,9 +70,12 @@ pub enum DemSlopeAlg { ZevenbergenThorne, } -impl Display for DemSlopeAlg { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - f.write_fmt(format_args!("{self:?}")) +impl DemSlopeAlg { + pub(crate) fn to_gdal_option(&self) -> &'static str { + match self { + DemSlopeAlg::Horn => "Horn", + DemSlopeAlg::ZevenbergenThorne => "ZevenbergenThorne", + } } } diff --git a/src/raster/processing/dem/slope.rs b/src/raster/processing/dem/slope.rs index f1e8b8fd..85269697 100644 --- a/src/raster/processing/dem/slope.rs +++ b/src/raster/processing/dem/slope.rs @@ -76,7 +76,7 @@ impl SlopeOptions { if let Some(alg) = self.algorithm { opts.add_string("-alg").unwrap(); - opts.add_string(&alg.to_string()).unwrap(); + opts.add_string(alg.to_gdal_option()).unwrap(); } if let Some(scale) = self.scale { diff --git a/src/raster/processing/dem/tri.rs b/src/raster/processing/dem/tri.rs index 57242fd8..e77c9bbd 100644 --- a/src/raster/processing/dem/tri.rs +++ b/src/raster/processing/dem/tri.rs @@ -1,4 +1,3 @@ -use std::fmt::{Display, Formatter}; use std::num::NonZeroUsize; use crate::cpl::CslStringList; @@ -41,7 +40,7 @@ impl TriOptions { #[cfg(all(major_is_3, minor_ge_3))] if let Some(alg) = self.algorithm { opts.add_string("-alg").unwrap(); - opts.add_string(&alg.to_string()).unwrap(); + opts.add_string(alg.to_gdal_option()).unwrap(); } opts @@ -66,9 +65,13 @@ pub enum DemTriAlg { Riley, } -impl Display for DemTriAlg { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - f.write_fmt(format_args!("{self:?}")) +impl DemTriAlg { + pub(crate) fn to_gdal_option(&self) -> &'static str { + match self { + DemTriAlg::Wilson => "Wilson", + #[cfg(all(major_is_3, minor_ge_3))] + DemTriAlg::Riley => "Riley", + } } } From e9af765b0cf6365c0d343700e07c286ec31a78aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lauren=C8=9Biu=20Nicola?= Date: Sat, 11 Nov 2023 11:55:14 +0200 Subject: [PATCH 3/8] Avoid unwrapping when building the dem options list --- src/raster/processing/dem/aspect.rs | 20 +++++++------ src/raster/processing/dem/color_relief.rs | 19 ++++++------- src/raster/processing/dem/hillshade.rs | 34 ++++++++++++----------- src/raster/processing/dem/mod.rs | 14 +++++----- src/raster/processing/dem/options.rs | 14 ++++++---- src/raster/processing/dem/roughness.rs | 12 ++++---- src/raster/processing/dem/slope.rs | 22 ++++++++------- src/raster/processing/dem/tpi.rs | 12 ++++---- src/raster/processing/dem/tri.rs | 13 +++++---- 9 files changed, 86 insertions(+), 74 deletions(-) diff --git a/src/raster/processing/dem/aspect.rs b/src/raster/processing/dem/aspect.rs index 0f43e3eb..cb427af3 100644 --- a/src/raster/processing/dem/aspect.rs +++ b/src/raster/processing/dem/aspect.rs @@ -1,7 +1,9 @@ +use std::num::NonZeroUsize; + use super::options::common_dem_options; use crate::cpl::CslStringList; +use crate::errors; use crate::raster::processing::dem::DemSlopeAlg; -use std::num::NonZeroUsize; /// Configuration options for [`aspect()`][super::aspect()]. #[derive(Debug, Clone, Default)] @@ -45,25 +47,25 @@ impl AspectOptions { /// Render relevant common options into [`CslStringList`] values, as compatible with /// [`gdal_sys::GDALDEMProcessing`]. - pub fn to_options_list(&self) -> CslStringList { + pub fn to_options_list(&self) -> errors::Result { let mut opts = CslStringList::default(); - self.store_common_options_to(&mut opts); + self.store_common_options_to(&mut opts)?; if let Some(alg) = self.algorithm { - opts.add_string("-alg").unwrap(); - opts.add_string(alg.to_gdal_option()).unwrap(); + opts.add_string("-alg")?; + opts.add_string(alg.to_gdal_option())?; } if self.zero_for_flat == Some(true) { - opts.add_string("-zero_for_flat").unwrap(); + opts.add_string("-zero_for_flat")?; } if self.trigonometric == Some(true) { - opts.add_string("-trigonometric").unwrap(); + opts.add_string("-trigonometric")?; } - opts + Ok(opts) } } @@ -93,7 +95,7 @@ mod tests { let expected: CslStringList = "-compute_edges -b 2 -of GTiff CPL_DEBUG=ON -alg ZevenbergenThorne -zero_for_flat -trigonometric" .parse()?; - assert_eq!(expected.to_string(), proc.to_options_list().to_string()); + assert_eq!(expected.to_string(), proc.to_options_list()?.to_string()); Ok(()) } diff --git a/src/raster/processing/dem/color_relief.rs b/src/raster/processing/dem/color_relief.rs index 230a8c9c..680f8a99 100644 --- a/src/raster/processing/dem/color_relief.rs +++ b/src/raster/processing/dem/color_relief.rs @@ -2,6 +2,7 @@ use std::num::NonZeroUsize; use std::path::{Path, PathBuf}; use crate::cpl::CslStringList; +use crate::errors; use crate::raster::processing::dem::options::common_dem_options; /// Configuration options for [`color_relief()`][super::color_relief()]. @@ -43,7 +44,7 @@ impl ColorReliefOptions { /// /// # Example /// Here's an example `.clr` file showing a number of the features described above. - /// + /// /// ```text /// 2600 white /// 2000 235 220 175 @@ -94,24 +95,22 @@ impl ColorReliefOptions { /// Render relevant common options into [`CslStringList`] values, as compatible with /// [`gdal_sys::GDALDEMProcessing`]. - pub fn to_options_list(&self) -> CslStringList { + pub fn to_options_list(&self) -> errors::Result { let mut opts = CslStringList::default(); - self.store_common_options_to(&mut opts); + self.store_common_options_to(&mut opts)?; if self.alpha == Some(true) { - opts.add_string("-alpha").unwrap(); + opts.add_string("-alpha")?; } match self.color_matching_mode { - ColorMatchingMode::ExactColorEntry => opts.add_string("-exact_color_entry").unwrap(), - ColorMatchingMode::NearestColorEntry => { - opts.add_string("-nearest_color_entry").unwrap() - } + ColorMatchingMode::ExactColorEntry => opts.add_string("-exact_color_entry")?, + ColorMatchingMode::NearestColorEntry => opts.add_string("-nearest_color_entry")?, _ => {} } - opts + Ok(opts) } } @@ -155,7 +154,7 @@ mod tests { let expected: CslStringList = "-compute_edges -b 2 -of GTiff CPL_DEBUG=ON -alpha -nearest_color_entry".parse()?; - assert_eq!(expected.to_string(), proc.to_options_list().to_string()); + assert_eq!(expected.to_string(), proc.to_options_list()?.to_string()); Ok(()) } diff --git a/src/raster/processing/dem/hillshade.rs b/src/raster/processing/dem/hillshade.rs index b25a7758..208f3274 100644 --- a/src/raster/processing/dem/hillshade.rs +++ b/src/raster/processing/dem/hillshade.rs @@ -1,7 +1,9 @@ +use std::num::NonZeroUsize; + use crate::cpl::CslStringList; +use crate::errors; use crate::raster::processing::dem::options::common_dem_options; use crate::raster::processing::dem::DemSlopeAlg; -use std::num::NonZeroUsize; /// Configuration options for [`hillshade()`][super::hillshade()]. #[derive(Debug, Clone, Default)] @@ -114,41 +116,41 @@ impl HillshadeOptions { /// Render relevant common options into [`CslStringList`] values, as compatible with /// [`gdal_sys::GDALDEMProcessing`]. - pub fn to_options_list(&self) -> CslStringList { + pub fn to_options_list(&self) -> errors::Result { let mut opts = CslStringList::default(); - self.store_common_options_to(&mut opts); + self.store_common_options_to(&mut opts)?; if let Some(alg) = self.algorithm { - opts.add_string("-alg").unwrap(); - opts.add_string(alg.to_gdal_option()).unwrap(); + opts.add_string("-alg")?; + opts.add_string(alg.to_gdal_option())?; } if let Some(scale) = self.scale { - opts.add_string("-s").unwrap(); - opts.add_string(&scale.to_string()).unwrap(); + opts.add_string("-s")?; + opts.add_string(&scale.to_string())?; } if let Some(mode) = self.shading { - opts.add_string(mode.to_gdal_option()).unwrap(); + opts.add_string(mode.to_gdal_option())?; } if let Some(factor) = self.z_factor { - opts.add_string("-z").unwrap(); - opts.add_string(&factor.to_string()).unwrap(); + opts.add_string("-z")?; + opts.add_string(&factor.to_string())?; } if let Some(altitude) = self.altitude { - opts.add_string("-alt").unwrap(); - opts.add_string(&altitude.to_string()).unwrap(); + opts.add_string("-alt")?; + opts.add_string(&altitude.to_string())?; } if let Some(azimuth) = self.azimuth { - opts.add_string("-az").unwrap(); - opts.add_string(&azimuth.to_string()).unwrap(); + opts.add_string("-az")?; + opts.add_string(&azimuth.to_string())?; } - opts + Ok(opts) } } @@ -210,7 +212,7 @@ mod tests { let expected: CslStringList = "-compute_edges -b 2 -of GTiff CPL_DEBUG=ON -alg ZevenbergenThorne -s 98473 -igor -z 2 -alt 45 -az 330" .parse()?; - assert_eq!(expected.to_string(), proc.to_options_list().to_string()); + assert_eq!(expected.to_string(), proc.to_options_list()?.to_string()); Ok(()) } diff --git a/src/raster/processing/dem/mod.rs b/src/raster/processing/dem/mod.rs index c817856f..16fd16b7 100644 --- a/src/raster/processing/dem/mod.rs +++ b/src/raster/processing/dem/mod.rs @@ -106,7 +106,7 @@ pub fn aspect>( ds, dest_file.as_ref(), DemAlg::Aspect, - &options.to_options_list(), + &options.to_options_list()?, None, ) } @@ -166,7 +166,7 @@ pub fn color_relief>( ds, dest_file.as_ref(), DemAlg::ColorRelief, - &options.to_options_list(), + &options.to_options_list()?, Some(colors), ) } @@ -224,7 +224,7 @@ pub fn hillshade>( ds, dest_file.as_ref(), DemAlg::Hillshade, - &options.to_options_list(), + &options.to_options_list()?, None, ) } @@ -274,7 +274,7 @@ pub fn roughness>( ds, dest_file.as_ref(), DemAlg::Roughness, - &options.to_options_list(), + &options.to_options_list()?, None, ) } @@ -333,7 +333,7 @@ pub fn slope>( ds, dest_file.as_ref(), DemAlg::Slope, - &options.to_options_list(), + &options.to_options_list()?, None, ) } @@ -383,7 +383,7 @@ pub fn topographic_position_index>( ds, dest_file.as_ref(), DemAlg::Tpi, - &options.to_options_list(), + &options.to_options_list()?, None, ) } @@ -433,7 +433,7 @@ pub fn terrain_ruggedness_index>( ds, dest_file.as_ref(), DemAlg::Tri, - &options.to_options_list(), + &options.to_options_list()?, None, ) } diff --git a/src/raster/processing/dem/options.rs b/src/raster/processing/dem/options.rs index 0b8e59a9..2c474672 100644 --- a/src/raster/processing/dem/options.rs +++ b/src/raster/processing/dem/options.rs @@ -149,24 +149,26 @@ macro_rules! common_dem_options { } /// Private utility to convert common options into [`CslStringList`] options. - fn store_common_options_to(&self, opts: &mut CslStringList) { + fn store_common_options_to(&self, opts: &mut CslStringList) -> errors::Result<()> { if self.compute_edges { - opts.add_string("-compute_edges").unwrap(); + opts.add_string("-compute_edges")?; } if let Some(band) = self.input_band { - opts.add_string("-b").unwrap(); - opts.add_string(&band.to_string()).unwrap(); + opts.add_string("-b")?; + opts.add_string(&band.to_string())?; } if let Some(of) = &self.output_format { - opts.add_string("-of").unwrap(); - opts.add_string(of).unwrap(); + opts.add_string("-of")?; + opts.add_string(of)?; } if !self.additional_options.is_empty() { opts.extend(&self.additional_options); } + + Ok(()) } }; } diff --git a/src/raster/processing/dem/roughness.rs b/src/raster/processing/dem/roughness.rs index 089c8418..2dc87c91 100644 --- a/src/raster/processing/dem/roughness.rs +++ b/src/raster/processing/dem/roughness.rs @@ -1,6 +1,8 @@ +use std::num::NonZeroUsize; + use crate::cpl::CslStringList; +use crate::errors; use crate::raster::processing::dem::options::common_dem_options; -use std::num::NonZeroUsize; /// Configuration options for [`roughness()`][super::roughness()]. @@ -22,10 +24,10 @@ impl RoughnessOptions { /// Render relevant options into [`CslStringList`] values, as compatible with /// [`gdal_sys::GDALDEMProcessing`]. - pub fn to_options_list(&self) -> CslStringList { + pub fn to_options_list(&self) -> errors::Result { let mut opts = CslStringList::new(); - self.store_common_options_to(&mut opts); - opts + self.store_common_options_to(&mut opts)?; + Ok(opts) } } @@ -49,7 +51,7 @@ mod tests { .with_additional_options("CPL_DEBUG=ON".parse()?); let expected: CslStringList = "-compute_edges -b 2 -of GTiff CPL_DEBUG=ON".parse()?; - assert_eq!(expected.to_string(), proc.to_options_list().to_string()); + assert_eq!(expected.to_string(), proc.to_options_list()?.to_string()); Ok(()) } diff --git a/src/raster/processing/dem/slope.rs b/src/raster/processing/dem/slope.rs index 85269697..6f44c633 100644 --- a/src/raster/processing/dem/slope.rs +++ b/src/raster/processing/dem/slope.rs @@ -1,7 +1,9 @@ +use std::num::NonZeroUsize; + use crate::cpl::CslStringList; +use crate::errors; use crate::raster::processing::dem::options::common_dem_options; use crate::raster::processing::dem::DemSlopeAlg; -use std::num::NonZeroUsize; /// Configuration options for [`slope()`][super::slope()]. #[derive(Debug, Clone, Default)] @@ -69,26 +71,26 @@ impl SlopeOptions { /// Render relevant common options into [`CslStringList`] values, as compatible with /// [`gdal_sys::GDALDEMProcessing`]. - pub fn to_options_list(&self) -> CslStringList { + pub fn to_options_list(&self) -> errors::Result { let mut opts = CslStringList::default(); - self.store_common_options_to(&mut opts); + self.store_common_options_to(&mut opts)?; if let Some(alg) = self.algorithm { - opts.add_string("-alg").unwrap(); - opts.add_string(alg.to_gdal_option()).unwrap(); + opts.add_string("-alg")?; + opts.add_string(alg.to_gdal_option())?; } if let Some(scale) = self.scale { - opts.add_string("-s").unwrap(); - opts.add_string(&scale.to_string()).unwrap(); + opts.add_string("-s")?; + opts.add_string(&scale.to_string())?; } if self.percentage_results == Some(true) { - opts.add_string("-p").unwrap(); + opts.add_string("-p")?; } - opts + Ok(opts) } } @@ -118,7 +120,7 @@ mod tests { let expected: CslStringList = "-compute_edges -b 2 -of GTiff CPL_DEBUG=ON -alg ZevenbergenThorne -s 98473 -p" .parse()?; - assert_eq!(expected.to_string(), proc.to_options_list().to_string()); + assert_eq!(expected.to_string(), proc.to_options_list()?.to_string()); Ok(()) } diff --git a/src/raster/processing/dem/tpi.rs b/src/raster/processing/dem/tpi.rs index 574a846e..ed9be5a6 100644 --- a/src/raster/processing/dem/tpi.rs +++ b/src/raster/processing/dem/tpi.rs @@ -1,6 +1,8 @@ +use std::num::NonZeroUsize; + use crate::cpl::CslStringList; +use crate::errors; use crate::raster::processing::dem::options::common_dem_options; -use std::num::NonZeroUsize; /// Configuration options for [`topographic_position_index()`][super::topographic_position_index()]. #[derive(Debug, Clone, Default)] @@ -21,10 +23,10 @@ impl TpiOptions { /// Render options into [`CslStringList`] values, as compatible with /// [`gdal_sys::GDALDEMProcessing`]. - pub fn to_options_list(&self) -> CslStringList { + pub fn to_options_list(&self) -> errors::Result { let mut opts = CslStringList::new(); - self.store_common_options_to(&mut opts); - opts + self.store_common_options_to(&mut opts)?; + Ok(opts) } } @@ -48,7 +50,7 @@ mod tests { .with_additional_options("CPL_DEBUG=ON".parse()?); let expected: CslStringList = "-compute_edges -b 2 -of GTiff CPL_DEBUG=ON".parse()?; - assert_eq!(expected.to_string(), proc.to_options_list().to_string()); + assert_eq!(expected.to_string(), proc.to_options_list()?.to_string()); Ok(()) } diff --git a/src/raster/processing/dem/tri.rs b/src/raster/processing/dem/tri.rs index e77c9bbd..71d011e2 100644 --- a/src/raster/processing/dem/tri.rs +++ b/src/raster/processing/dem/tri.rs @@ -1,6 +1,7 @@ use std::num::NonZeroUsize; use crate::cpl::CslStringList; +use crate::errors; use crate::raster::processing::dem::options::common_dem_options; /// Configuration options for [`terrain_ruggedness_index()`][super::terrain_ruggedness_index()]. @@ -29,21 +30,21 @@ impl TriOptions { /// Render relevant common options into [`CslStringList`] values, as compatible with /// [`gdal_sys::GDALDEMProcessing`]. - pub fn to_options_list(&self) -> CslStringList { + pub fn to_options_list(&self) -> errors::Result { let mut opts = CslStringList::default(); - self.store_common_options_to(&mut opts); + self.store_common_options_to(&mut opts)?; // Before 3.3, Wilson is the only algorithm and therefore there's no // selection option. Rust caller can still specify Wilson, but // we don't pass it along. #[cfg(all(major_is_3, minor_ge_3))] if let Some(alg) = self.algorithm { - opts.add_string("-alg").unwrap(); - opts.add_string(alg.to_gdal_option()).unwrap(); + opts.add_string("-alg")?; + opts.add_string(alg.to_gdal_option())?; } - opts + Ok(opts) } } @@ -99,7 +100,7 @@ mod tests { let expected: CslStringList = "-compute_edges -b 2 -of GTiff CPL_DEBUG=ON -alg Wilson".parse()?; - assert_eq!(expected.to_string(), opts.to_options_list().to_string()); + assert_eq!(expected.to_string(), opts.to_options_list()?.to_string()); Ok(()) } From 998a53877ddc77db2a95586608a342e62cf27e9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lauren=C8=9Biu=20Nicola?= Date: Sat, 11 Nov 2023 13:56:01 +0200 Subject: [PATCH 4/8] Reduce visibility of to_gdal_options --- src/raster/processing/dem/hillshade.rs | 2 +- src/raster/processing/dem/options.rs | 4 ++-- src/raster/processing/dem/tri.rs | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/raster/processing/dem/hillshade.rs b/src/raster/processing/dem/hillshade.rs index 208f3274..56431871 100644 --- a/src/raster/processing/dem/hillshade.rs +++ b/src/raster/processing/dem/hillshade.rs @@ -174,7 +174,7 @@ pub enum ShadingMode { } impl ShadingMode { - pub(crate) fn to_gdal_option(&self) -> &'static str { + fn to_gdal_option(self) -> &'static str { match self { ShadingMode::Combined => "-combined", ShadingMode::Multidirectional => "-multidirectional", diff --git a/src/raster/processing/dem/options.rs b/src/raster/processing/dem/options.rs index 2c474672..382a50b2 100644 --- a/src/raster/processing/dem/options.rs +++ b/src/raster/processing/dem/options.rs @@ -47,7 +47,7 @@ pub enum DemAlg { } impl DemAlg { - pub(crate) fn to_gdal_option(&self) -> &'static str { + pub(super) fn to_gdal_option(self) -> &'static str { match self { DemAlg::Aspect => "aspect", DemAlg::ColorRelief => "color-relief", @@ -71,7 +71,7 @@ pub enum DemSlopeAlg { } impl DemSlopeAlg { - pub(crate) fn to_gdal_option(&self) -> &'static str { + pub(super) fn to_gdal_option(self) -> &'static str { match self { DemSlopeAlg::Horn => "Horn", DemSlopeAlg::ZevenbergenThorne => "ZevenbergenThorne", diff --git a/src/raster/processing/dem/tri.rs b/src/raster/processing/dem/tri.rs index 71d011e2..4fb565ed 100644 --- a/src/raster/processing/dem/tri.rs +++ b/src/raster/processing/dem/tri.rs @@ -67,7 +67,7 @@ pub enum DemTriAlg { } impl DemTriAlg { - pub(crate) fn to_gdal_option(&self) -> &'static str { + fn to_gdal_option(self) -> &'static str { match self { DemTriAlg::Wilson => "Wilson", #[cfg(all(major_is_3, minor_ge_3))] From 3d0c812464bbd97624743d64cde15f10c5c88bfd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lauren=C8=9Biu=20Nicola?= Date: Sat, 11 Nov 2023 14:04:29 +0200 Subject: [PATCH 5/8] Don't clone in output_format --- src/raster/processing/dem/options.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/raster/processing/dem/options.rs b/src/raster/processing/dem/options.rs index 382a50b2..8782b70e 100644 --- a/src/raster/processing/dem/options.rs +++ b/src/raster/processing/dem/options.rs @@ -119,8 +119,8 @@ macro_rules! common_dem_options { } /// Fetch the specified output format driver identifier. - pub fn output_format(&self) -> Option { - self.output_format.clone() + pub fn output_format(&self) -> Option<&str> { + self.output_format.as_ref().map(|f| f.as_str()) } /// Compute values at image edges. From 85971803c1c5b0af51fd21eedbed8ceeaa6d018e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lauren=C8=9Biu=20Nicola?= Date: Sat, 11 Nov 2023 19:41:19 +0200 Subject: [PATCH 6/8] Clarify comment --- src/raster/processing/dem/tri.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/raster/processing/dem/tri.rs b/src/raster/processing/dem/tri.rs index 4fb565ed..f5195bf0 100644 --- a/src/raster/processing/dem/tri.rs +++ b/src/raster/processing/dem/tri.rs @@ -36,8 +36,8 @@ impl TriOptions { self.store_common_options_to(&mut opts)?; // Before 3.3, Wilson is the only algorithm and therefore there's no - // selection option. Rust caller can still specify Wilson, but - // we don't pass it along. + // selection option. + // Callers can still specify Wilson, but we don't pass it along. #[cfg(all(major_is_3, minor_ge_3))] if let Some(alg) = self.algorithm { opts.add_string("-alg")?; From 4b2242d5c410cf4a9d9036fad893c420f3b43365 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lauren=C8=9Biu=20Nicola?= Date: Mon, 13 Nov 2023 22:25:51 +0200 Subject: [PATCH 7/8] Remove dem options getters --- src/raster/processing/dem/color_relief.rs | 10 ++------- src/raster/processing/dem/hillshade.rs | 27 ----------------------- src/raster/processing/dem/mod.rs | 4 ++-- src/raster/processing/dem/options.rs | 20 ----------------- src/raster/processing/dem/slope.rs | 12 ---------- 5 files changed, 4 insertions(+), 69 deletions(-) diff --git a/src/raster/processing/dem/color_relief.rs b/src/raster/processing/dem/color_relief.rs index 680f8a99..6a85464f 100644 --- a/src/raster/processing/dem/color_relief.rs +++ b/src/raster/processing/dem/color_relief.rs @@ -75,11 +75,6 @@ impl ColorReliefOptions { self } - /// Get path to the color relief configuration file. - pub fn color_config(&self) -> PathBuf { - self.color_config.to_owned() - } - /// Specify the color matching mode. /// /// See [`ColorMatchingMode`] for details. @@ -88,9 +83,8 @@ impl ColorReliefOptions { self } - /// Get the color matching mode to be used. - pub fn color_matching_mode(&self) -> ColorMatchingMode { - self.color_matching_mode + pub(crate) fn color_config(&self) -> &Path { + &self.color_config } /// Render relevant common options into [`CslStringList`] values, as compatible with diff --git a/src/raster/processing/dem/hillshade.rs b/src/raster/processing/dem/hillshade.rs index 56431871..da62347e 100644 --- a/src/raster/processing/dem/hillshade.rs +++ b/src/raster/processing/dem/hillshade.rs @@ -34,11 +34,6 @@ impl HillshadeOptions { self } - /// Fetch the specified slope computation algorithm - pub fn algorithm(&self) -> Option { - self.algorithm - } - /// Specify the altitude of the light, in degrees. /// /// `90` if the light comes from above the DEM, `0` if it is raking light. @@ -47,11 +42,6 @@ impl HillshadeOptions { self } - /// Fetch the specified light altitude, in degrees. - pub fn altitude(&self) -> Option { - self.altitude - } - /// Specify the azimuth of the light, in degrees: /// /// * `0` if it comes from the top of the raster, @@ -83,13 +73,6 @@ impl HillshadeOptions { self } - /// Fetch the specified scaling factor. - /// - /// Returns `None` if one has not been previously set vai [`Self::with_scale`]. - pub fn scale(&self) -> Option { - self.scale - } - /// Specify the shading mode to render with. /// /// See [`ShadingMode`] for mode descriptions. @@ -98,22 +81,12 @@ impl HillshadeOptions { self } - /// Fetch the specified shading mode. - pub fn shading_mode(&self) -> Option { - self.shading - } - /// Vertical exaggeration used to pre-multiply the elevations pub fn with_z_factor(&mut self, z_factor: f64) -> &mut Self { self.z_factor = Some(z_factor); self } - /// Fetch the applied z-factor value. - pub fn z_factor(&self) -> Option { - self.z_factor - } - /// Render relevant common options into [`CslStringList`] values, as compatible with /// [`gdal_sys::GDALDEMProcessing`]. pub fn to_options_list(&self) -> errors::Result { diff --git a/src/raster/processing/dem/mod.rs b/src/raster/processing/dem/mod.rs index 16fd16b7..3b02a72c 100644 --- a/src/raster/processing/dem/mod.rs +++ b/src/raster/processing/dem/mod.rs @@ -23,7 +23,7 @@ //! use std::ffi::{CStr, CString}; -use std::path::{Path, PathBuf}; +use std::path::Path; use std::ptr; use libc::c_int; @@ -444,7 +444,7 @@ fn dem_eval( dst_file: &Path, alg: DemAlg, options: &CslStringList, - color_relief_config: Option, + color_relief_config: Option<&Path>, ) -> Result { let popts = options::GdalDEMProcessingOptions::new(options)?; let mode = CString::new(alg.to_gdal_option())?; diff --git a/src/raster/processing/dem/options.rs b/src/raster/processing/dem/options.rs index 8782b70e..ffeff7c8 100644 --- a/src/raster/processing/dem/options.rs +++ b/src/raster/processing/dem/options.rs @@ -89,11 +89,6 @@ macro_rules! common_dem_options { self } - /// Fetch the specified input band to read from. - pub fn input_band(&self) -> Option { - self.input_band - } - /// Explicitly specify output raster format. /// /// This is equivalent to the `-of ` CLI flag accepted by many GDAL tools. @@ -118,11 +113,6 @@ macro_rules! common_dem_options { self } - /// Fetch the specified output format driver identifier. - pub fn output_format(&self) -> Option<&str> { - self.output_format.as_ref().map(|f| f.as_str()) - } - /// Compute values at image edges. /// /// If true, causes interpolation of values at image edges or if a no-data value is found @@ -132,22 +122,12 @@ macro_rules! common_dem_options { self } - /// Fetch the compute edges mode. - pub fn compute_edges(&self) -> bool { - self.compute_edges - } - /// Additional generic options to be included. pub fn with_additional_options(&mut self, extra_options: CslStringList) -> &mut Self { self.additional_options.extend(&extra_options); self } - /// Fetch additional options. - pub fn additional_options(&self) -> &CslStringList { - &self.additional_options - } - /// Private utility to convert common options into [`CslStringList`] options. fn store_common_options_to(&self, opts: &mut CslStringList) -> errors::Result<()> { if self.compute_edges { diff --git a/src/raster/processing/dem/slope.rs b/src/raster/processing/dem/slope.rs index 6f44c633..93e61053 100644 --- a/src/raster/processing/dem/slope.rs +++ b/src/raster/processing/dem/slope.rs @@ -31,11 +31,6 @@ impl SlopeOptions { self } - /// Fetch the specified slope computation algorithm. - pub fn algorithm(&self) -> Option { - self.algorithm - } - /// Apply a elevation scaling factor. /// /// Routine assumes x, y and z units are identical. @@ -54,13 +49,6 @@ impl SlopeOptions { self } - /// Fetch the specified scaling factor. - /// - /// Returns `None` if one has not been previously set via [`Self::with_scale`]. - pub fn scale(&self) -> Option { - self.scale - } - /// If `state` is `true`, the slope will be expressed as percent slope. /// /// Otherwise, it is expressed as degrees From e78c54215ce0c60bd997434f906202bf58181be4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lauren=C8=9Biu=20Nicola?= Date: Tue, 14 Nov 2023 16:49:41 +0200 Subject: [PATCH 8/8] Make compute_edges an Option again --- src/raster/processing/dem/aspect.rs | 2 +- src/raster/processing/dem/color_relief.rs | 4 ++-- src/raster/processing/dem/hillshade.rs | 2 +- src/raster/processing/dem/options.rs | 4 ++-- src/raster/processing/dem/roughness.rs | 2 +- src/raster/processing/dem/slope.rs | 2 +- src/raster/processing/dem/tpi.rs | 2 +- src/raster/processing/dem/tri.rs | 2 +- 8 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/raster/processing/dem/aspect.rs b/src/raster/processing/dem/aspect.rs index cb427af3..20cd9f9f 100644 --- a/src/raster/processing/dem/aspect.rs +++ b/src/raster/processing/dem/aspect.rs @@ -9,7 +9,7 @@ use crate::raster::processing::dem::DemSlopeAlg; #[derive(Debug, Clone, Default)] pub struct AspectOptions { input_band: Option, - compute_edges: bool, + compute_edges: Option, output_format: Option, additional_options: CslStringList, algorithm: Option, diff --git a/src/raster/processing/dem/color_relief.rs b/src/raster/processing/dem/color_relief.rs index 6a85464f..7597c6f8 100644 --- a/src/raster/processing/dem/color_relief.rs +++ b/src/raster/processing/dem/color_relief.rs @@ -9,7 +9,7 @@ use crate::raster::processing::dem::options::common_dem_options; #[derive(Debug, Clone)] pub struct ColorReliefOptions { input_band: Option, - compute_edges: bool, + compute_edges: Option, output_format: Option, additional_options: CslStringList, color_config: PathBuf, @@ -58,7 +58,7 @@ impl ColorReliefOptions { pub fn new>(color_config: P) -> Self { Self { input_band: None, - compute_edges: false, + compute_edges: None, output_format: None, additional_options: Default::default(), color_config: color_config.as_ref().to_path_buf(), diff --git a/src/raster/processing/dem/hillshade.rs b/src/raster/processing/dem/hillshade.rs index da62347e..a72db0bc 100644 --- a/src/raster/processing/dem/hillshade.rs +++ b/src/raster/processing/dem/hillshade.rs @@ -9,7 +9,7 @@ use crate::raster::processing::dem::DemSlopeAlg; #[derive(Debug, Clone, Default)] pub struct HillshadeOptions { input_band: Option, - compute_edges: bool, + compute_edges: Option, output_format: Option, additional_options: CslStringList, algorithm: Option, diff --git a/src/raster/processing/dem/options.rs b/src/raster/processing/dem/options.rs index ffeff7c8..b56fa647 100644 --- a/src/raster/processing/dem/options.rs +++ b/src/raster/processing/dem/options.rs @@ -118,7 +118,7 @@ macro_rules! common_dem_options { /// If true, causes interpolation of values at image edges or if a no-data value is found /// in the 3x3 processing window. pub fn with_compute_edges(&mut self, state: bool) -> &mut Self { - self.compute_edges = state; + self.compute_edges = Some(state); self } @@ -130,7 +130,7 @@ macro_rules! common_dem_options { /// Private utility to convert common options into [`CslStringList`] options. fn store_common_options_to(&self, opts: &mut CslStringList) -> errors::Result<()> { - if self.compute_edges { + if matches!(self.compute_edges, Some(true)) { opts.add_string("-compute_edges")?; } diff --git a/src/raster/processing/dem/roughness.rs b/src/raster/processing/dem/roughness.rs index 2dc87c91..64d9332a 100644 --- a/src/raster/processing/dem/roughness.rs +++ b/src/raster/processing/dem/roughness.rs @@ -9,7 +9,7 @@ use crate::raster::processing::dem::options::common_dem_options; #[derive(Debug, Clone, Default)] pub struct RoughnessOptions { input_band: Option, - compute_edges: bool, + compute_edges: Option, output_format: Option, additional_options: CslStringList, } diff --git a/src/raster/processing/dem/slope.rs b/src/raster/processing/dem/slope.rs index 93e61053..bb9890b5 100644 --- a/src/raster/processing/dem/slope.rs +++ b/src/raster/processing/dem/slope.rs @@ -9,7 +9,7 @@ use crate::raster::processing::dem::DemSlopeAlg; #[derive(Debug, Clone, Default)] pub struct SlopeOptions { input_band: Option, - compute_edges: bool, + compute_edges: Option, output_format: Option, additional_options: CslStringList, algorithm: Option, diff --git a/src/raster/processing/dem/tpi.rs b/src/raster/processing/dem/tpi.rs index ed9be5a6..d5218762 100644 --- a/src/raster/processing/dem/tpi.rs +++ b/src/raster/processing/dem/tpi.rs @@ -8,7 +8,7 @@ use crate::raster::processing::dem::options::common_dem_options; #[derive(Debug, Clone, Default)] pub struct TpiOptions { input_band: Option, - compute_edges: bool, + compute_edges: Option, output_format: Option, additional_options: CslStringList, } diff --git a/src/raster/processing/dem/tri.rs b/src/raster/processing/dem/tri.rs index f5195bf0..a7ba31ec 100644 --- a/src/raster/processing/dem/tri.rs +++ b/src/raster/processing/dem/tri.rs @@ -8,7 +8,7 @@ use crate::raster::processing::dem::options::common_dem_options; #[derive(Debug, Clone, Default)] pub struct TriOptions { input_band: Option, - compute_edges: bool, + compute_edges: Option, output_format: Option, additional_options: CslStringList, algorithm: Option,