Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unified raster creation options over CplStringList. #519

Merged
merged 1 commit into from
Feb 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
# Changes

## Unreleased

- **Breaking** Removed `RasterCreationOption`and replaced usages of `[RasterCreationOption]` with `RasterCreationOptions`, a type alias for `CplStringList`.

- <https://github.com/georust/gdal/pull/519>

- Add `DriverIterator` format to iterate through drivers, as well as `DriverManager::all()` method that provides the iterator.
- <https://github.com/georust/gdal/pull/512>

Expand Down
13 changes: 4 additions & 9 deletions src/dataset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use gdal_sys::{self, CPLErr, GDALDatasetH, GDALMajorObjectH};
use crate::cpl::CslStringList;
use crate::errors::*;
use crate::options::DatasetOptions;
use crate::raster::RasterCreationOption;
use crate::raster::RasterCreationOptions;
use crate::utils::{_last_cpl_err, _last_null_pointer_err, _path_to_c_string, _string};
use crate::{
gdal_major_object::MajorObject, spatial_ref::SpatialRef, Driver, GeoTransform, Metadata,
Expand Down Expand Up @@ -238,28 +238,23 @@ impl Dataset {
&self,
driver: &Driver,
filename: P,
options: &[RasterCreationOption],
options: &RasterCreationOptions,
) -> Result<Dataset> {
fn _create_copy(
ds: &Dataset,
driver: &Driver,
filename: &Path,
options: &[RasterCreationOption],
options: &CslStringList,
) -> Result<Dataset> {
let c_filename = _path_to_c_string(filename)?;

let mut c_options = CslStringList::new();
for option in options {
c_options.set_name_value(option.key, option.value)?;
}

let c_dataset = unsafe {
gdal_sys::GDALCreateCopy(
driver.c_driver(),
c_filename.as_ptr(),
ds.c_dataset,
0,
c_options.as_ptr(),
options.as_ptr(),
None,
ptr::null_mut(),
)
Expand Down
28 changes: 8 additions & 20 deletions src/driver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,10 @@ use std::sync::Once;

use gdal_sys::{self, CPLErr, GDALDriverH, GDALMajorObjectH};

use crate::cpl::CslStringList;
use crate::dataset::Dataset;
use crate::gdal_major_object::MajorObject;
use crate::metadata::Metadata;
use crate::raster::{GdalDataType, GdalType, RasterCreationOption};
use crate::raster::{GdalDataType, GdalType, RasterCreationOptions};
use crate::utils::{_last_cpl_err, _last_null_pointer_err, _path_to_c_string, _string};

use crate::errors::*;
Expand Down Expand Up @@ -137,7 +136,7 @@ impl Driver {
size_y: usize,
bands: usize,
) -> Result<Dataset> {
let options = [];
let options = Default::default();
self.create_with_band_type_with_options::<T, _>(filename, size_x, size_y, bands, &options)
}

Expand All @@ -153,16 +152,11 @@ impl Driver {
/// ```rust, no_run
/// # fn main() -> gdal::errors::Result<()> {
/// use gdal::DriverManager;
/// use gdal::raster::RasterCreationOption;
/// use gdal::raster::RasterCreationOptions;
/// use gdal::raster::GdalDataType;
/// use gdal::spatial_ref::SpatialRef;
/// let d = DriverManager::get_driver_by_name("BMP")?;
/// let options = [
/// RasterCreationOption {
/// key: "WORLDFILE",
/// value: "YES"
/// }
/// ];
/// let options = RasterCreationOptions::from_iter(["WORLD_FILE=YES"]);
/// let mut ds = d.create_with_band_type_with_options::<u8, _>("/tmp/foo.bmp", 64, 64, 1, &options)?;
/// ds.set_spatial_ref(&SpatialRef::from_epsg(4326)?)?;
/// assert_eq!(ds.raster_count(), 1);
Expand All @@ -178,7 +172,7 @@ impl Driver {
size_x: usize,
size_y: usize,
bands: usize,
options: &[RasterCreationOption],
options: &RasterCreationOptions,
) -> Result<Dataset> {
Self::_create_with_band_type_with_options(
self,
Expand All @@ -198,13 +192,8 @@ impl Driver {
size_y: usize,
bands: usize,
data_type: GdalDataType,
options: &[RasterCreationOption],
options: &RasterCreationOptions,
) -> Result<Dataset> {
let mut options_c = CslStringList::new();
for option in options {
options_c.set_name_value(option.key, option.value)?;
}

let size_x = libc::c_int::try_from(size_x)?;
let size_y = libc::c_int::try_from(size_y)?;
let bands = libc::c_int::try_from(bands)?;
Expand All @@ -218,7 +207,7 @@ impl Driver {
size_y,
bands,
data_type as u32,
options_c.as_ptr(),
options.as_ptr(),
)
};

Expand All @@ -232,14 +221,13 @@ impl Driver {
/// Convenience for creating a vector-only dataset from a compatible driver.
/// [Details](https://gdal.org/api/gdaldriver_cpp.html#_CPPv4N10GDALDriver6CreateEPKciii12GDALDataType12CSLConstList)
pub fn create_vector_only<P: AsRef<Path>>(&self, filename: P) -> Result<Dataset> {
let options = [];
self._create_with_band_type_with_options(
filename.as_ref(),
0,
0,
0,
GdalDataType::Unknown,
&options,
&RasterCreationOptions::default(),
)
}

Expand Down
7 changes: 7 additions & 0 deletions src/raster/create_options.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
use crate::cpl::CslStringList;

/// Key/value pairs of options for passing driver-specific creation flags to
/// [`Driver::create_with_band_type_with_options`](crate::Driver::create_with_band_type_with_options`).
///
/// See `papszOptions` in [GDAL's `Create(...)` API documentation](https://gdal.org/api/gdaldriver_cpp.html#_CPPv4N10GDALDriver6CreateEPKciii12GDALDataType12CSLConstList).
pub type RasterCreationOptions = CslStringList;
Copy link
Contributor Author

@metasim metasim Jan 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% sure if

a) A type alias is a good idea
b) It should be in a separate file

Rationale:

a) If we ever needed to convert it into a newtype (to add creation-specific methods like validate, it's less breaking. Also, it's easy to get create options and other options confused, so this makes it a bit more clear.
b) Putting a single type in mod.rs feels unbalanced.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a) If we ever needed to convert it into a newtype (to add creation-specific methods like validate, it's less breaking. Also, it's easy to get create options and other options confused, so this makes it a bit more clear.

We should probably re-export it from the parent module, so not breaking.

b) Putting a single type in mod.rs feels unbalanced.

There might be more 🙂.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably re-export it from the parent module, so not breaking.

It is:

https://github.com/georust/gdal/pull/519/files#diff-eb172c9758a6ca0ee77f152d6a8c864de3a68ea11439bb26fa33ea60d3490155R78

12 changes: 2 additions & 10 deletions src/raster/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@
//! ```

pub use buffer::{Buffer, ByteBuffer};
pub use create_options::RasterCreationOptions;
#[cfg(all(major_ge_3, minor_ge_1))]
pub use mdarray::{
Attribute, Dimension, ExtendedDataType, ExtendedDataTypeClass, Group, MDArray, MdStatisticsAll,
Expand All @@ -88,6 +89,7 @@ pub use types::{AdjustedValue, GdalDataType, GdalType};
pub use warp::reproject;

mod buffer;
mod create_options;
#[cfg(all(major_ge_3, minor_ge_1))]
mod mdarray;
pub mod processing;
Expand All @@ -97,13 +99,3 @@ mod rasterize;
mod tests;
mod types;
mod warp;

/// Key/value pair for passing driver-specific creation options to
/// [`Driver::create_with_band_type_wth_options`](crate::Driver::create_with_band_type_with_options`).
///
/// See `papszOptions` in [GDAL's `Create(...)` API documentation](https://gdal.org/api/gdaldriver_cpp.html#_CPPv4N10GDALDriver6CreateEPKciii12GDALDataType12CSLConstList).
#[derive(Debug)]
pub struct RasterCreationOption<'a> {
pub key: &'a str,
pub value: &'a str,
}
23 changes: 6 additions & 17 deletions src/raster/rasterband.rs
Original file line number Diff line number Diff line change
Expand Up @@ -578,23 +578,12 @@ impl<'a> RasterBand<'a> {
/// ```rust, no_run
/// # fn main() -> gdal::errors::Result<()> {
/// use gdal::DriverManager;
/// use gdal::raster::{Buffer, RasterCreationOption};
/// use gdal::raster::{Buffer, RasterCreationOptions };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

/// use gdal::raster::{Buffer, RasterCreationOptions };

///
/// let driver = DriverManager::get_driver_by_name("GTiff").unwrap();
/// let options = [
/// RasterCreationOption {
/// key: "TILED",
/// value: "YES",
/// },
/// RasterCreationOption {
/// key: "BLOCKXSIZE",
/// value: "16",
/// },
/// RasterCreationOption {
/// key: "BLOCKYSIZE",
/// value: "16",
/// },
/// ];
/// let options = RasterCreationOptions::from_iter([
/// "TILED=YES", "BLOCKXSIZE=16", "BLOCKYSIZE=16"
/// ]);
/// let dataset = driver
/// .create_with_band_type_with_options::<u16, _>(
/// "/vsimem/test_write_block.tif",
Expand Down Expand Up @@ -1434,7 +1423,7 @@ impl Debug for ColorEntry {
///
/// // Create in-memory copy to mutate
/// let mem_driver = DriverManager::get_driver_by_name("MEM")?;
/// let ds = ds.create_copy(&mem_driver, "<mem>", &[])?;
/// let ds = ds.create_copy(&mem_driver, "<mem>", &Default::default())?;
/// let mut band = ds.rasterband(1)?;
/// assert!(band.color_table().is_none());
///
Expand All @@ -1448,7 +1437,7 @@ impl Debug for ColorEntry {
///
/// // Render a PNG
/// let png_driver = DriverManager::get_driver_by_name("PNG")?;
/// ds.create_copy(&png_driver, "/tmp/labels.png", &[])?;
/// ds.create_copy(&png_driver, "/tmp/labels.png", &Default::default())?;
///
/// # Ok(())
/// # }
Expand Down
67 changes: 17 additions & 50 deletions src/raster/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use crate::dataset::Dataset;
use crate::metadata::Metadata;
use crate::raster::rasterband::ResampleAlg;
use crate::raster::{
ByteBuffer, ColorEntry, ColorInterpretation, ColorTable, GdalDataType, RasterCreationOption,
ByteBuffer, ColorEntry, ColorInterpretation, ColorTable, GdalDataType, RasterCreationOptions,
StatisticsAll, StatisticsMinMax,
};
use crate::test_utils::{fixture, TempFixture};
Expand Down Expand Up @@ -127,7 +127,9 @@ fn test_rename_remove_raster() {

let driver = DriverManager::get_driver_by_name("GTiff").unwrap();

dataset.create_copy(&driver, mem_file_path_a, &[]).unwrap();
dataset
.create_copy(&driver, mem_file_path_a, &Default::default())
.unwrap();

driver.rename(mem_file_path_b, mem_file_path_a).unwrap();

Expand Down Expand Up @@ -166,28 +168,13 @@ fn test_create_with_band_type() {
#[test]
fn test_create_with_band_type_with_options() {
let driver = DriverManager::get_driver_by_name("GTiff").unwrap();
let options = [
RasterCreationOption {
key: "TILED",
value: "YES",
},
RasterCreationOption {
key: "BLOCKXSIZE",
value: "128",
},
RasterCreationOption {
key: "BLOCKYSIZE",
value: "64",
},
RasterCreationOption {
key: "COMPRESS",
value: "LZW",
},
RasterCreationOption {
key: "INTERLEAVE",
value: "BAND",
},
];
let options = RasterCreationOptions::from_iter([
"TILED=YES",
"BLOCKXSIZE=128",
"BLOCKYSIZE=64",
"COMPRESS=LZW",
"INTERLEAVE=BAND",
]);

let tmp_filename = TempFixture::empty("test.tif");
{
Expand All @@ -214,7 +201,9 @@ fn test_create_with_band_type_with_options() {
fn test_create_copy() {
let driver = DriverManager::get_driver_by_name("MEM").unwrap();
let dataset = Dataset::open(fixture("tinymarble.tif")).unwrap();
let copy = dataset.create_copy(&driver, "", &[]).unwrap();
let copy = dataset
.create_copy(&driver, "", &Default::default())
.unwrap();
assert_eq!(copy.raster_size(), (100, 50));
assert_eq!(copy.raster_count(), 3);
}
Expand All @@ -234,16 +223,7 @@ fn test_create_copy_with_options() {
.create_copy(
&DriverManager::get_driver_by_name("GTiff").unwrap(),
mem_file_path,
&[
RasterCreationOption {
key: "INTERLEAVE",
value: "BAND",
},
RasterCreationOption {
key: "COMPRESS",
value: "LZW",
},
],
&RasterCreationOptions::from_iter(["INTERLEAVE=BAND", "COMPRESS=LZW"]),
)
.unwrap();

Expand Down Expand Up @@ -391,20 +371,7 @@ fn test_read_block_data() {
#[cfg(feature = "ndarray")]
fn test_write_block() {
let driver = DriverManager::get_driver_by_name("GTiff").unwrap();
let options = [
RasterCreationOption {
key: "TILED",
value: "YES",
},
RasterCreationOption {
key: "BLOCKXSIZE",
value: "16",
},
RasterCreationOption {
key: "BLOCKYSIZE",
value: "16",
},
];
let options = RasterCreationOptions::from_iter(["TILED=YES", "BLOCKXSIZE=16", "BLOCKYSIZE=16"]);
let dataset = driver
.create_with_band_type_with_options::<u16, _>(
"/vsimem/test_write_block.tif",
Expand Down Expand Up @@ -724,7 +691,7 @@ fn test_create_color_table() {

// Create a new file to put color table in
let dataset = dataset
.create_copy(&dataset.driver(), &outfile, &[])
.create_copy(&dataset.driver(), &outfile, &Default::default())
.unwrap();
dataset
.rasterband(1)
Expand Down
Loading